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

Option to error on missing postBuild substitution variable #4694

Closed
1 task done
sjdrc opened this issue Mar 26, 2024 · 9 comments · Fixed by fluxcd/kustomize-controller#1130
Closed
1 task done

Option to error on missing postBuild substitution variable #4694

sjdrc opened this issue Mar 26, 2024 · 9 comments · Fixed by fluxcd/kustomize-controller#1130

Comments

@sjdrc
Copy link

sjdrc commented Mar 26, 2024

Describe the bug

I'm currently trying to convert some helmfiles into flux manifests.

We currently use environment values to allow us to configure an entire application stack, and allow users to install everything according to our configuration but with setting their own required inputs. As helmfile uses gotemplates for generating the final manifests we can do logic such as requiring environment values be set (with {{ required "You must provide a value for someValue!" .Values.someValue }}).

It looks like flux post build variable substitution will do the trick for us, and the bash string replacement functions are useful, but I'm quite surprised that by default an unset variable will just evaluate to an empty string. I want to set up flux manifests that require these values to be set and returns an error if you do not.

The kubernetes cluster-api project have an open issue discussing switching to an alternative implementation of envsubst, but there is not feature parity between the two projects.

Would it be possible to add an option to flux to error if any of the variables are not set? (Or evaluate to an empty string)

Steps to reproduce

N/A

Expected behavior

Able to enforce users to provide values for postBuild.substitute

Screenshots and recordings

No response

OS / Distro

N/A

Flux version

N/A

Flux check

N/A

Git provider

No response

Container Registry provider

No response

Additional context

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@souleb
Copy link
Member

souleb commented Mar 26, 2024

I looked at the linked issue.

If I'm not wrong with the proposed replacement we are going to lose a couple of the syntax supported by the current library (https://github.com/drone/envsubst vs https://github.com/a8m/envsubst#docs), this will be breaking for the users.

I agree with this, we don't want to break our users setup, this could result in major pain in order to replace whole setups.

I think the best would be for drone/envsubst#34 to land.

@sjdrc
Copy link
Author

sjdrc commented Mar 26, 2024

I think the best would be for drone/envsubst#34 to land.

That repo's last update is over 3 and a half years old and they've disabled their issue tracker (and it's that PR's birthday today!). Looks to be unmaintained.

I did find this fork though https://github.com/gomodules/envsubst that actually already has this functionality added to it!

Would you consider switching to use this package instead? If so I can open a PR.

@stefanprodan
Copy link
Member

Would you consider switching to use this package instead?

Let's see if @tamalsaha is keen on maintaining it going forward.

@tamalsaha
Copy link

Yes, we maintain that project and use it internally.

@stefanprodan
Copy link
Member

stefanprodan commented Mar 26, 2024

@sjdrc we could switch to gomodules/envsubst and add a feature flag to kustomize-controller that when set, would fail the reconciliation when vars are missing. We also need to look into what other changes are in gomodules/envsubst and make sure they don't change the behaviour. Given that kustomize-controller is GA, any behaviour changes to substitutions must be opt-in.

@sjdrc
Copy link
Author

sjdrc commented Mar 27, 2024

That would be excellent and would fulfill my use case perfectly!

@stefanprodan
Copy link
Member

@sjdrc can you help implementing this?

@sjdrc
Copy link
Author

sjdrc commented Mar 27, 2024

@stefanprodan
I'm not too familiar with Go, but I'm happy to give it a try over the long weekend!

@stefanprodan
Copy link
Member

@sjdrc I've read all the code changes in https://github.com/gomodules/envsubst and we can't use. The PR from upstream wasn't ported, instead this fork errors out for undefined vars which breaks Flux.

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 a pull request may close this issue.

4 participants