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

Support lazy configuration for mainClass and jvmFlags properties of ContainerParameters #3936

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

erdi
Copy link
Contributor

@erdi erdi commented Feb 22, 2023

This is a fix for #3927. I tried to follow the changes for similar issues applied in #3242 and #3709 as suggested in #3927 (comment).

If there is anything that needs to be changed before this PR can be accepted then please just let me know and I will amend it as necessary.

Before filing a pull request, make sure to do the following:

This helps to reduce the chance of having a pull request rejected.

@erdi erdi force-pushed the fix-3927 branch 2 times, most recently from a516384 to ff72655 Compare February 22, 2023 17:05
@erdi erdi changed the title Support lazy configuration for mainClass property of ContainerParameters Support lazy configuration for mainClass and jvmFlags properties of ContainerParameters Feb 22, 2023
@erdi erdi marked this pull request as ready for review February 22, 2023 17:11
@erdi erdi changed the title Support lazy configuration for mainClass and jvmFlags properties of ContainerParameters Support lazy configuration for mainClass and jvmFlags properties of ContainerParameters Feb 22, 2023
@mpeddada1 mpeddada1 self-assigned this Feb 23, 2023
@erdi
Copy link
Contributor Author

erdi commented Apr 3, 2023

Are there plans to review this PR in the near future, please?

Copy link
Contributor

@emmileaf emmileaf left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution and apologies for the delay in review! The changes look great in general - could you help add a note on this enhancement in the README.md as well?

(Also - no idea why the presubmit checks are stuck, going to close and re-open this PR to see if they re-trigger.)

@emmileaf
Copy link
Contributor

emmileaf commented Apr 3, 2023

Regarding the kokoro failures - they don't look related to the changes in this PR, and should be resolved by updating the branch against main to include fixes from #3965. Thanks!

@erdi
Copy link
Contributor Author

erdi commented Apr 4, 2023

Regarding the kokoro failures - they don't look related to the changes in this PR, and should be resolved by updating the branch against main to include fixes from #3965. Thanks!

That's done, thanks.

@erdi
Copy link
Contributor Author

erdi commented Apr 4, 2023

could you help add a note on this enhancement in the README.md as well?

I've added notes to README.md about the properties now being lazily configurable similarly to the notes added in #3709.

@erdi
Copy link
Contributor Author

erdi commented Apr 4, 2023

I believe that all comments have been addressed so back to you, @emmileaf.

Copy link
Contributor

@emmileaf emmileaf left a comment

Choose a reason for hiding this comment

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

@erdi Awesome, thank you so much for the contribution!

(Quick note that the presubmit check error is from #3978 and not related to changes from this PR; we should be able to merge once that is unblocked.)

@erdi
Copy link
Contributor Author

erdi commented Apr 6, 2023

Thanks @emmileaf. I've registered for notifications from #3978 and #3979 so when they get merged/resolved I will pull main into this branch. Hopefully this will make it easier for you.

@erdi
Copy link
Contributor Author

erdi commented Apr 9, 2023

Latest master, which includes #3979, has been merged into this branch now so this is ready for another run of checks, @emmileaf.

@sonarcloud
Copy link

sonarcloud bot commented Apr 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

90.0% 90.0% Coverage
0.0% 0.0% Duplication

@emmileaf
Copy link
Contributor

Thanks again @erdi!

@emmileaf emmileaf merged commit 8f99f9e into GoogleContainerTools:master Apr 11, 2023
@erdi erdi deleted the fix-3927 branch April 12, 2023 07:33
@erdi
Copy link
Contributor Author

erdi commented Apr 12, 2023

And thank you for reviewing and merging, @emmileaf!

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.

4 participants