Conversation
bobbyiliev
force-pushed
the
initial-setup
branch
2 times, most recently
from
9964e9e to
84ac4f2
Compare
.github/workflows/shell-tests.yml
Outdated
| - name: Run ShellCheck | ||
| uses: ludeeus/action-shellcheck@master | ||
| with: | ||
| severity: error | ||
| check_together: "yes" | ||
| formats: sh |
There was a problem hiding this comment.
NIT
I'd just move this to a bin/lint script that can get called... you can check out i2's pre-commit for example, but doing this via local script call that also installs necessary packages makes it way easier to use locally.
There was a problem hiding this comment.
Whoa there! unit testing bash!?
@alex-hunt-materialize is gonna love this.
.github/workflows/shell-tests.yml
Outdated
| git clone https://github.com/bats-core/bats-support.git /tmp/bats-support | ||
| git clone https://github.com/bats-core/bats-assert.git /tmp/bats-assert | ||
| git clone https://github.com/bats-core/bats-file.git /tmp/bats-file |
There was a problem hiding this comment.
might want to lock these versions down .
manage-taints.sh
Outdated
| echo "Removing taint $TAINT_KEY from node $NODE_NAME" | ||
|
|
||
| kubectl --server="https://${KUBERNETES_SERVICE_HOST}:${KUBERNETES_SERVICE_PORT}" \ | ||
| --token="$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \ |
There was a problem hiding this comment.
Does kubectl not automatically detect this?
There was a problem hiding this comment.
Oh good point, I'll need to check this.
configure-disks.sh
Outdated
| jq -r '.blockdevices[] | select(.model // empty | contains("Amazon EC2 NVMe Instance Storage")) | .path' 2>/dev/null) && [ -n "$lsblk_output" ]; then | ||
| echo "$lsblk_output" | ||
| else | ||
| nvme list | grep "Amazon EC2 NVMe Instance Storage" | awk '{print $1}' |
There was a problem hiding this comment.
Why do we need both ways here?
configure-disks.sh
Outdated
| echo "$CLOUD_PROVIDER" | ||
| return 0 | ||
| else | ||
| echo "ERROR: CLOUD_PROVIDER environment variable is required" >&2 |
There was a problem hiding this comment.
You are setting this as a bash variable above, when it gets passed as an arg. If they set it as an env var, it won't work. Please tell them to pass the --cloud-provider arg instead.
configure-disks.sh
Outdated
|
|
||
| echo "Starting NVMe disk configuration..." | ||
|
|
||
| if ! CLOUD_PROVIDER=$(detect_cloud_provider); then |
There was a problem hiding this comment.
| if ! CLOUD_PROVIDER=$(detect_cloud_provider); then | |
| if [[ -z "$CLOUD_PROVIDER" ]]; then |
I see no need for detect_cloud_provider at all if you aren't doing any detection.
manage-taints.sh
Outdated
| echo "Usage: $0 [remove]" | ||
| exit 1 | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Why not call the script remove-taint.sh and not have to parse this arg?
manage-taints.sh
Outdated
|
|
||
| TAINT_KEY="disk-unconfigured" | ||
|
|
||
| if [ -z "${NODE_NAME:-}" ]; then |
There was a problem hiding this comment.
Nitpick, it's a bit weird to mix env var parsing and args.
manage-taints.sh
Outdated
| if [ -z "$KUBERNETES_SERVICE_HOST" ] || [ -z "$KUBERNETES_SERVICE_PORT" ]; then | ||
| echo "Error: Kubernetes service environment variables not found" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ ! -f "/var/run/secrets/kubernetes.io/serviceaccount/token" ]; then | ||
| echo "Error: Service account token not found" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
kubectl should automatically detect this stuff, and possibly other auth methods. No need to manually check this.
manage-taints.sh
Outdated
| ;; | ||
| esac | ||
|
|
||
| exit 0 |
There was a problem hiding this comment.
exit 0 at the end of a script is almost always redundant. Better to remove it, so if you forget set -e you don't accidentally hide a failure.
bobbyiliev
force-pushed
the
initial-setup
branch
from
ff912fe to
5cb0070
Compare
3 participants
I've also started preparing a PR for the GCP Terraform module here: MaterializeInc/terraform-google-materialize#44