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.

Sign up for GitHub

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

Jump to bottom

Properly support defining env vars from secret/configmap keys #9758

Merged
merged 1 commit into from
Jun 11, 2020
Merged

Properly support defining env vars from secret/configmap keys #9758

merged 1 commit into from
Jun 11, 2020

Conversation

metacosm
Copy link
Contributor

@metacosm metacosm commented Jun 3, 2020 •
edited
Loading

Fixes #9629

  • Restore (undocumented) behavior that was broken with the new style configuration
  • Provide new style configuration for defining env vars from secret/configmap keys
  • Check/improve validation
  • Add integration tests
  • Update documentation

@metacosm metacosm mentioned this pull request Jun 4, 2020
Closed
@metacosm metacosm marked this pull request as ready for review June 4, 2020 22:44
@metacosm metacosm force-pushed the fix-9629 branch 2 times, most recently from b3cb6ee to 8f531a9 Compare June 7, 2020 20:28
@metacosm
Copy link
Contributor Author

metacosm commented Jun 7, 2020

@iocanel this is ready for review

@iocanel iocanel self-requested a review June 7, 2020 22:18
iocanel
iocanel requested changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

Looks great.

The only thing I'd like to be added is to slightly extend the coverage. See details in the comments.

...rnetes/quarkus-standard-way/src/test/resources/kubernetes-with-env-from-configmap.properties Show resolved Hide resolved
...ubernetes/quarkus-standard-way/src/test/resources/kubernetes-with-env-from-secret.properties Show resolved Hide resolved
@metacosm metacosm requested a review from iocanel June 9, 2020 13:23
@geoand
Copy link
Contributor

geoand commented Jun 9, 2020

@metacosm is this ready? If so, we should try and get it in tomorrow's release.

@iocanel can you please review?

@metacosm
Copy link
Contributor Author

metacosm commented Jun 9, 2020

@metacosm is this ready? If so, we should try and get it in tomorrow's release.

@iocanel can you please review?

That's ready from my side. I just need to squash the last commits. One thing I'm not too sure about are the vars-from-configmap and vars-from-secret names which kind of are a mouthful so I'm willing to change them if people have better names for this… :)

@geoand
Copy link
Contributor

geoand commented Jun 9, 2020

I absolutely suck at naming, so I won't be able to provide any better one :)

@geoand
Copy link
Contributor

geoand commented Jun 9, 2020

@metacosm can you please squash the commits in order to make the commit ready for merging?

@metacosm
Copy link
Contributor Author

metacosm commented Jun 9, 2020

@geoand done

@geoand
Copy link
Contributor

geoand commented Jun 9, 2020

Thanks!

iocanel
iocanel reviewed Jun 10, 2020
View reviewed changes
...ubernetes/quarkus-standard-way/src/test/resources/kubernetes-with-env-from-secret.properties Outdated
@@ -1 +1,3 @@
quarkus.kubernetes.env-vars.ignored.secret=my-secret
quarkus.kubernetes.env.secrets=my-secret
quarkus.kubernetes.env.vars-from-secret.db-password.secret=db-secret
Copy link
Contributor

Choose a reason for hiding this comment

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

@metacosm: Since you've asked about naming ... How about

quarkus.kubernetes.env.secret.db-password.secret=db-secret
quarkus.kubernetes.env.secret.db-password.key=database.password

Copy link
Contributor

Choose a reason for hiding this comment

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

Since, the property that lets you pass one or more secrets is called secrets I feel that this has to be aligned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would be too confusing / ambiguous so I'd rather have something explicit. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since, the property that lets you pass one or more secrets is called secrets I feel that this has to be aligned.

So what would you suggest then?

@metacosm
Copy link
Contributor Author

metacosm commented Jun 11, 2020 •
edited
Loading

Decision was made to use

quarkus.kubernetes.env.mapping.db-password.from-secret=db-secret
quarkus.kubernetes.env.mapping.db-password.with-key=database.password

syntax. @iocanel @geoand ready for a final review, if you please! 😄

@geoand
Copy link
Contributor

geoand commented Jun 11, 2020

@iocanel please review since you've been following more than me

@metacosm metacosm requested a review from iocanel June 11, 2020 13:27
iocanel
iocanel approved these changes Jun 11, 2020
View reviewed changes
Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

LGTM

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 11, 2020
@metacosm
Copy link
Contributor Author

I guess I didn't go through the Java 14 tests… 🤷

@geoand
Copy link
Contributor

geoand commented Jun 11, 2020 •
edited
Loading

Java 14 has been a thorn in our side the past few days when it comes to CI...

@geoand geoand merged commit 2383a1f into quarkusio:master Jun 11, 2020
@metacosm
Copy link
Contributor Author

Java 14 has been a thorn in our side the past few days when it comes to CI...

Yep, I know 😉

@gsmet gsmet added this to the 1.6.0 - master milestone Jun 25, 2020
@manusa manusa mentioned this pull request Oct 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iocanel iocanel iocanel approved these changes

Assignees
No one assigned
Labels
area/documentation area/kubernetes triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.6.0.CR1
Development

Successfully merging this pull request may close these issues.

Kubernetes env vars generation changed unexpectedly
4 participants
@metacosm @geoand @iocanel @gsmet