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

manifest envsubst #329

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

Conversation

so-jelly
Copy link

@so-jelly so-jelly commented Oct 25, 2021

What this PR does / why we need it:

This PR replaces variable references in manifests with environment variables. It also adds the generated pet name to the environment so that it can be templated as well.

Fixes # #203

pkg/test/utils/translate.go Outdated Show resolved Hide resolved
pkg/test/utils/translate.go Outdated Show resolved Hide resolved
pkg/test/utils/translate_test.go Outdated Show resolved Hide resolved
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a deeper look later today... however I would move out of test/utils. It was there prior to my getting involved but have always disliked it. We don't use "utils" in Go generally. the package name provides meaning and context to the use of a func or struct. Could we please consider moving it to a more meaningful package?

We are not going to introduce a need copyright into our code base tho... so translate.go needs to change to a deps or be written.

@kensipe
Copy link
Member

kensipe commented Feb 13, 2022

Thanks for the contribution... We need a sign-off on the DCO to move forward

@gberche-orange
Copy link
Contributor

gberche-orange commented Oct 27, 2022

thanks @so-jelly for this contrib! It would very much helm me with asserting cluster-wide resources dynamically instanciated by a kuttl and referencing the associated $NAMESPACE.

Any chance for you to proceed with the DCO signature so that this feature can be merged,, such as running git rebase --signoff HEAD~2 ?

@haarchri
Copy link

haarchri commented Jan 7, 2023

@so-jelly can you rebase and add DCO - happy to see this change comes in ;)

Signed-off-by: Shawn O'Dell <[email protected]>
so-jelly and others added 5 commits January 22, 2023 17:03
Co-authored-by: Chris Bandy <[email protected]>
Signed-off-by: Shawn O'Dell <[email protected]>
Signed-off-by: Shawn O'Dell <[email protected]>
Signed-off-by: Shawn O'Dell <[email protected]>
Co-authored-by: Chris Bandy <[email protected]>
Signed-off-by: Shawn O'Dell <[email protected]>
Co-authored-by: Chris Bandy <[email protected]>
Signed-off-by: Shawn O'Dell <[email protected]>
@so-jelly
Copy link
Author

I refactored to remove that licensed code. Some edge cases i considered is variables in GVK and keys. I have thought through this a bit and I think it is sound. I can add some test cases as well.

@so-jelly so-jelly requested review from cbandy and kensipe and removed request for kaiwalyajoshi, asekretenko and cbandy January 23, 2023 17:54
@so-jelly so-jelly requested review from cbandy and kensipe and removed request for kensipe and cbandy January 23, 2023 23:38
@so-jelly
Copy link
Author

I created test cases for some additional field entries. I am not sure what is up with Github review requests. tagging @kensipe in case the review request isn't sufficient.

Copy link

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this basically yesterday :) Any chance we can see this merged?

pkg/test/case.go Outdated Show resolved Hide resolved
Signed-off-by: Shawn O'Dell <[email protected]>
@kensipe
Copy link
Member

kensipe commented Jun 26, 2023

@jaypipes I can provide details elsewhere... I'm just seeing this. looks urgent and apologies for being out of pocket... for way too long. I will review today/night and respond and/or merge

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.

None yet

6 participants