stardog-union / helm-charts

Stardog Helm Charts
Apache License 2.0
9 stars 12 forks source link

Make imagePullSecrets optional #77

Closed Dridge closed 1 year ago

Dridge commented 2 years ago

Make imagePullSecrets optional, in order to make both the busybox and main stardog image pull secrets optional and avoid excessive errors in journalctl cmd.

Jul 18 09:07:52 re-1.relocal k3s[3559]: I0718 09:07:52.913214    3559 kubelet_pods.go:891] "Unable to retrieve pull secret, the image pull may not succeed." pod="re-graph/richard-stardog-0" secret="" err="secret \"richard-image-pull-secret\" not found"
Jul 18 09:07:52 re-1.relocal k3s[3559]: I0718 09:07:52.913337    3559 kubelet_pods.go:891] "Unable to retrieve pull secret, the image pull may not succeed." pod="re-graph/richard-stardog-0" secret="" err="secret \"richard-image-busybox-pull-secret\" not found"

Possibly the whole imagePullSecrets needs to be inside an if statement too? But helm linting didn't throw any errors with the below change.

Testing Currently only tested with helm lint.

igable commented 2 years ago

@Dridge thanks for this. Image pull secrets should definitely be made optional as you suggest. We will need to test this a bit. I'll get back to you with a proper review once we have had a chance.

paulplace commented 1 year ago

@Dridge Apologies for the delay in responding here. We now have our CircleCI checks for this repo working, and we need to run them against your PR before merging. When you have a moment, please pull the latest commits from this repo and rebase your commits onto the develop branch. Also, if you have an extra moment, please add a test for your change in the tests subdirectory.

Dridge commented 1 year ago

I have had some trouble trying to write test for stateful sets and journalctl messages, can anyone else assist or contribute to this? I'm not familiar with circleci either. Do we need that to pass before it can be merged?

scardena commented 1 year ago

Hey @Dridge , for some reason circleci didn't pick up your branch. I just restarted the job in circle and your branch is passing the current tests, so it looks like your changes are good to go. you don't really need to go beyond and write complex for this, but thing I'd ask, is just to add a simple helm lint same as you are doing locally to verify that the helm chart is valid. you could add that validation in here: https://github.com/stardog-union/helm-charts/blob/f9b8eba89803a125b4faf6058298ab86a9655ea9/tests/smoke.sh#L14 a function like:

function validate_helm_chart() {
    echo "Validating the helm chart"
    helm lint charts/stardog >/dev/null 2>&1 || { echo >&2 "The helm chart is not valid, exiting."; exit 1; }
    echo "Helm chart valid."
}

and then call the function at the beggining of https://github.com/stardog-union/helm-charts/blob/f9b8eba89803a125b4faf6058298ab86a9655ea9/tests/smoke.sh#L245

echo "Starting the Helm smoke tests"
validate_helm_chart

Also, please squash your changes or rebase your changes on top of our latest develop. that way is easier to identify which are your changes.

Dridge commented 1 year ago

Here is a test idea I thought of but didn't confirm works. Just add it here in case it's something worthwhile implementing. helm-chart-test-secret-message.patch

function check_logs_do_not_contain_image_pull_secret() {
  echo "Test that the logs do not 'image-pull-secret\" not found'"
  kubectl exec journalctl -e | grep -n "Unable to retrieve pull secret, the image pull may not succeed."
  rc=$?

  if [ ${rc} -ne 1 ]; then
    echo "Stardog logs contained the unexpected log message, failed test, exiting"
    exit ${rc}
  fi
  echo "No image-pull-secret messages found, passed test"
}

echo "Test: image pull secrets no longer complain in logs"
helm_install_single_node_stardog
check_logs_do_not_contain_image_pull_secret
Dridge commented 1 year ago

Squashed my commits down to one, rebasing was always forcing me to create repeated merge commits so squashing seemed easier and makes the PR look cleaner.