Skip to content

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

Merged
emmileaf merged 5 commits intoGoogleContainerTools:masterfrom
erdi:fix-3927
Apr 11, 2023
Merged

Support lazy configuration for mainClass and jvmFlags properties of ContainerParameters#3936
emmileaf merged 5 commits intoGoogleContainerTools:masterfrom
erdi:fix-3927

Conversation

@erdi
Copy link
Contributor

@erdi erdi commented Feb 22, 2023 •
edited
Loading

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 added a commit to erdi/jib that referenced this pull request Feb 22, 2023
@erdi
Support lazy configuration for jvmFlags property of ContainerParameters
a516384 
Fixes GoogleContainerTools#3936.
@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?

emmileaf
emmileaf reviewed Apr 3, 2023
View reviewed changes
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 emmileaf closed this Apr 3, 2023
@emmileaf emmileaf reopened this Apr 3, 2023
@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.

@erdi erdi requested a review from emmileaf April 4, 2023 14:48
emmileaf
emmileaf approved these changes Apr 5, 2023
View reviewed changes
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.

@sonarqubecloud
Copy link

sonarqubecloud 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

emmileaf commented Apr 11, 2023

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!

@erdi erdi mentioned this pull request May 12, 2023
Closed
@emmileaf emmileaf mentioned this pull request May 15, 2023
Closed
@emmileaf emmileaf mentioned this pull request Jun 8, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@emmileaf emmileaf emmileaf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mpeddada1 mpeddada1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erdi @emmileaf @kokoro-team @mpeddada1