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

fix(master): pass lifecycle configuration to master container #1062

Merged
merged 13 commits into from
Nov 22, 2024

Conversation

DionJones615
Copy link

@DionJones615 DionJones615 commented Sep 23, 2024

Changes

Simply passed the lifecycle defined in the Jenkins CRD down to the generated pod definition. The necessary bits appear to already be in the CRD.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS.

Release Notes

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

@DionJones615 DionJones615 changed the title Ns 3390 fix(master): pass lifecycle configuration to master container Sep 23, 2024
@brokenpip3
Copy link
Collaborator

ehi @DionJones615 thanks for your contribution.

I think here we miss:

  • a guard if lifecycle is empty
  • the option listed in the helm chart values and jenkins template

wdyt?

thanks

@DionJones615 DionJones615 marked this pull request as draft November 11, 2024 02:38
@DionJones615 DionJones615 marked this pull request as ready for review November 12, 2024 12:48
@DionJones615
Copy link
Author

@brokenpip3 Sorry, priorities shifted for a minute. :) How's this looking? Working well in my tests

@brokenpip3
Copy link
Collaborator

Nice, ty for coming back and try to complete the PR!

did you tried locally with a binary build from this PR to pass a lifecycle? I wish we can add a test, at least here. If everything is green after that we can merge it :)

@DionJones615
Copy link
Author

Nice, ty for coming back and try to complete the PR!

did you tried locally with a binary build from this PR to pass a lifecycle? I wish we can add a test, at least here. If everything is green after that we can merge it :)

Yep, we're about to push it to prod to improve the shutdown process, keeping the jenkins container open with quietDown enabled for better communication with users about shutdown processes, especially when the backup/restore takes a bit of time.

Test added 🤞

Copy link
Collaborator

@brokenpip3 brokenpip3 left a comment

Choose a reason for hiding this comment

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

perfect, thanks for your contribution!

@brokenpip3 brokenpip3 merged commit d919a2a into jenkinsci:master Nov 22, 2024
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.

2 participants