-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ Template defaults with envsubst #3270
✨ Template defaults with envsubst #3270
Conversation
/test pull-cluster-api-e2e |
/milestone v0.3.x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wfernandes the PR looks great to me (few small nits)
it will be great to update the clusterct provider contract adding a note about the legacy format (support will be dropped in future) and providing a table of supported syntax for variables (something similar to this but with a brief explanation for each function)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good, some nits on the doc
915d0db
to
2f14a19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this @wfernandes! I will pull your branch and give this a try with some Azure var defaults
@wfernandes to clarify, |
/test pull-cluster-api-e2e |
@CecileRobertMichon I tested it out and I believe that will work. I built the binary locally and did the following. $ export CLUSTER_NAME=foobar
$ echo "AzureVnetName: ${AZURE_VNET_NAME:-${CLUSTER_NAME}-vnet}" | ./envsubst
AzureVnetName: foobar-vnet I can add a test case if that would help. |
@CecileRobertMichon So it seems that I misspoke. As I was writing the test for the above using the library, the use case you mentioned above was failing. |
2f14a19
to
5f9fe82
Compare
That'd be a nice addition, let's see if we can add support for it — it should be as easy as having a commit merged to the library and vendoring it, or do we need other special code for it? |
/hold Working on a fix to envsubst to support syntax like |
I created this PR drone/envsubst#23 to add support for the syntax discussed above. However, I've also updated this PR with a go.mod replace directive to use my fork temporarily. Once upstream drone/envsubst has been merged and tagged, we can switch to that. @CecileRobertMichon Feel free to test this out and let me know if this works for you. 🙂 /hold cancel |
@wfernandes In general, we should wait for the library PR to merge, let's ping the envsubst maintainers to see if they can review and get it merged within the next two weeks |
/hold |
@fabriziopandini @CecileRobertMichon @wfernandes Do we want to try to get this in v0.3.7 or wait? |
I'm +1 to get this merged with the current version of drone if the new won't get ready, because it will greatly simplify the usage of experimental features by introducing something like
|
Just to confirm, this allows defining defaults for the provider components as well? if so, +1 to getting this in from me as well to be able to test ClusterResourceSet more easily as Fabrizio points out. |
Does it? 🤔 |
This is a change to the Also I got feedback on the drone/envsubst#23 PR so I'll update that and hopefully it will be merged soonish. 🤷 |
Also FYI, @vincepri Currently there is a replace directive pointing to my fork of envsubst so that others could pull this down and test it. If this is a "no-no" please let me "know-now". 😆 |
If the PR doesn't get merged in the envsubst library tomorrow or early next week, we should probably remove the fork and add the support later |
0501ce8
to
c1f6c15
Compare
@CecileRobertMichon I did a quick test using the overrides functionality of clusterctl and verified that defaults work for |
Awesome! envsubst PR just merged 🎉 |
@vincepri Lol...as I said that, it just got merged! drone/envsubst#23 |
- This is change to the SimpleProcessor so defaults will be available to cluster templates and provider components. - Create specific errMissingVariables error Since we are using a map, the order of the variables in the error message was changing. This would make the tests flaky since we were doing a fixed substring comparison. So the error which wraps the slice of missing vars is used - Use a specific sha of drone/envsubst until a tag is cut.
c1f6c15
to
d4907ef
Compare
I squashed the commits...let me know if there is any other feedback! |
/hold cancel |
/lgtm |
as: `${ VAR }`, `${ VAR}`,`${VAR }`. However, these formats will be deprecated | ||
in the near future. e.g. v1alpha4. | ||
|
||
Formats such as `${VAR$FOO}` is not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this supported now? @CecileRobertMichon @wfernandes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not supported. There has to be an expression character like =
, :=
, :-
between the variables. This will result in an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/milestone v0.3.7
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR introduces support for default variables in the SimpleProcessor. It leverages the library https://github.com/drone/envsubst to parse and evaluate the yaml.
Since the library by default doesn't support
${ VAR}, ${ VAR }, ${VAR }, ${VA$R}
, we've added backward compatibility for the first three, however we've dropped support for${VA$R}
.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #3189