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

hack/release: Remove ttlSecondsAfterFinished from certgen job #4200

Merged

Conversation

lrewega
Copy link
Contributor

@lrewega lrewega commented Nov 29, 2021

The certgen job is versioned in releases, so deleting it should not be
necessary for most consumers. Deleting the job can cause problems when
applying the quickstart config via continous deployment (e.g. Argo CD)
as the job disappears before an external observer can acknowledge it
completed successfully, or even confirm that it ever existed.

Signed-off-by: Luke Rewega lrewega@buf.build

@lrewega lrewega requested a review from a team as a code owner November 29, 2021 22:19
@lrewega lrewega requested review from stevesloka and sunjayBhatia and removed request for a team November 29, 2021 22:19
@lrewega
Copy link
Contributor Author

lrewega commented Nov 29, 2021

cc @stevesloka @youngnick as discussed on Slack a few hours ago

I did some very minor testing (./hack/release/make-release-tag.sh 1.19.0 42.42.42) but any further testing by maintainers would be appreciated.

@lrewega lrewega force-pushed the lrewega/release-rm-certgen-job-ttl branch from f0239c1 to ea2aed4 Compare November 29, 2021 22:28
@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #4200 (ee80483) into main (39a31e6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4200   +/-   ##
=======================================
  Coverage   76.41%   76.41%           
=======================================
  Files         111      111           
  Lines        9883     9883           
=======================================
  Hits         7552     7552           
  Misses       2164     2164           
  Partials      167      167           

@sunjayBhatia sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Nov 30, 2021
@sunjayBhatia
Copy link
Member

@lrewega could you please add a release note following the process here: https://github.com/projectcontour/contour/blob/main/CONTRIBUTING.md#commit-message-and-pr-guidelines (labeled this as a "small" change)

thanks!

@lrewega lrewega force-pushed the lrewega/release-rm-certgen-job-ttl branch 2 times, most recently from 72c2962 to f49c5cf Compare November 30, 2021 21:36
@youngnick
Copy link
Member

Just the codespell issue to fix, then this LGTM. Nice work @lrewega, thanks.

@lrewega lrewega force-pushed the lrewega/release-rm-certgen-job-ttl branch 2 times, most recently from 18a0874 to ab39d14 Compare November 30, 2021 23:07
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM with one more spelling nit, sorry!

changelogs/unreleased/4200-lrewega-small.md Outdated Show resolved Hide resolved
The certgen job is versioned in releases, so deleting it should not be
necessary for most consumers. Deleting the job can cause problems when
applying the quickstart config via continous deployment (e.g. Argo CD)
as the job disappears before an external observer can acknowledge it
completed successfully, or even confirm that it ever existed.

Signed-off-by: Luke Rewega <lrewega@buf.build>
@lrewega lrewega force-pushed the lrewega/release-rm-certgen-job-ttl branch from a7f8597 to ee80483 Compare December 1, 2021 03:24
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @lrewega!

@sunjayBhatia sunjayBhatia merged commit 116b51c into projectcontour:main Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants