Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use yq for initscript #61

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

snowpoke
Copy link
Contributor

This PR makes it such that during the add-secret-values-to-config initContainer step, instead of using the debian:bookworm-slim image and then installing yq onto it, we use the mikefarah/yq:4 image right away.

My main motivation for this change was that this plays nicer with NetworkPolicies, as the current version requires the user to provide wide-ranging egress access rights to the MAS pod. (And also, it took me a very long time to figure out why the settings didn't populate properly, since the add-secret-values-to-config container currently doesn't exit on error - this change is likely going to save NetworkPolicy users a lot of troubleshooting)

@jessebot
Copy link
Collaborator

Sorry for the delay. I'm taking a look at this now. In the future, please bump the helm chart version, so that the linter works properly. Have you tested this locally?

@jessebot
Copy link
Collaborator

looking further at this, I think there was a reason we weren't using the yq image directly 🤔 I can't remember what it was though. Can you please bump the helm chart version and test locally that everything works?

@snowpoke
Copy link
Contributor Author

Hi! I'm actually using my fork of this repo in production

Maybe an issue you were facing was related to missing file permissions? I had to add a block that makes it respect my securityContext

(and yes, will bump version in the future! sorry for any trouble)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants