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

Add weekly line, retire lines older than 2.289.x #671

Merged
merged 9 commits into from
Oct 20, 2021
Merged

Add weekly line, retire lines older than 2.289.x #671

merged 9 commits into from
Oct 20, 2021

Conversation

timja
Copy link
Member

@timja timja commented Oct 5, 2021 •
edited
Loading

Fixes #492
Fixes #681

I'll add a separate PR to auto update the version if this is accepted.

This will make less work for LTS versions as we can detect disruptive core changes earlier

@timja timja added the enhancement New feature or request label Oct 5, 2021
@timja
Copy link
Member Author

timja commented Oct 5, 2021

Odd: https://github.com/jenkinsci/bom/pull/671/checks?check_run_id=3807537327

@timja timja requested a review from jglick October 5, 2021 22:01
jglick
jglick approved these changes Oct 5, 2021
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Looking forward to this!

bom-latest/pom.xml
@@ -6,7 +6,7 @@
<artifactId>parent</artifactId>
<version>${changelist}</version>
</parent>
<artifactId>bom-2.303.x</artifactId>
<artifactId>bom-weekly</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename the module to bom-weekly as well? If so, do not forget

bom/.github/dependabot.yml

Line 14 in 7c549ea

directory: "/bom-latest"

sample-plugin/pom.xml Outdated
<bom>2.303.x</bom>
<jenkins.version>2.303.1</jenkins.version>
<bom>weekly</bom>
<jenkins.version>2.314</jenkins.version>
Copy link
Member

Choose a reason for hiding this comment

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

So how do you propose to update this? Via Dependabot or some other mechanism?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably use @olblak ’s update cli

Copy link
Member

Choose a reason for hiding this comment

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

https://www.updatecli.io/docs/automate/github_action/ or https://www.updatecli.io/docs/automate/jenkins/

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah should be quite straightforward if it supports it, otherwise something like https://github.com/jenkinsci/helm-charts/blob/main/.github/workflows/sync-lts.yaml should work just fine too.

ideally fix dependabot, maybe worth looking briefly into it

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, updatecli doesn't support updating xml files at this stage. I started working on that a while ago and then unprioritized that.

Copy link
Member

@olblak olblak Oct 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

but we could easily implement a small shell script to handle that last operation similar to what we did with AMI configuration until I finish the work on xml

Copy link
Member

Choose a reason for hiding this comment

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

@olblak maybe simple text replacement would suffice?

@timja
Copy link
Member Author

timja commented Oct 7, 2021

Odd: #671 (checks)

it seems that bom-2.263 it not getting the version overrides from bom-2.277

jglick
jglick approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

it seems that bom-2.263 it not getting the version overrides from bom-2.277

Did you figure out why?

Needs merge with master.

local-test.sh
-e MAVEN_OPTS=-Duser.home=/var/maven \
-e MAVEN_CONFIG=/var/maven/.m2 \
-e PLUGINS=$PLUGINS \
-e PLUGINS="$PLUGINS" \
Copy link
Member

Choose a reason for hiding this comment

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

Fine though should not be necessary since this is a comma-separated line of ids, no shell metacharacters.

@timja
Copy link
Member Author

timja commented Oct 7, 2021

Did you figure out why?

nope still not figured it out, was just merging with master in my last push

@timja timja mentioned this pull request Oct 18, 2021
Closed
@timja
Copy link
Member Author

timja commented Oct 20, 2021

This started failing in 2.307, works before then.

@timja
Copy link
Member Author

timja commented Oct 20, 2021

Looks like it works if I upgrade git / git-server, which requires dropping the lines before 2.289

@timja timja changed the title Add weekly line Add weekly line, retire lines older than 2.289.x Oct 20, 2021
jglick
jglick approved these changes Oct 20, 2021
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Looks OK if you can get everything passing. Do not forget to cut a release from trunk prior to merging and releasing this, so that we have the last update available for now-dropped lines.

@jglick
Copy link
Member

jglick commented Oct 20, 2021

https://github.com/jenkinsci/bom/blob/master/README.md#lts-lines ought to be updated also.

@timja timja merged commit 8225607 into jenkinsci:master Oct 20, 2021
@timja timja deleted the add-weekly-line branch October 20, 2021 15:33
@jglick
Copy link
Member

jglick commented Oct 20, 2021

Nice work, thanks!

This was referenced Oct 20, 2021
Closed
Closed
@offa offa mentioned this pull request Oct 21, 2021
Closed
6 tasks
@jglick jglick mentioned this pull request Oct 27, 2021
Merged
@basil
Copy link
Member

basil commented Mar 17, 2022

This PR seems to have unintentionally invalidated the logic in

bom/Jenkinsfile

Line 35 in 4333236

lines = [lines[0], lines[-1]] // run PCT only on newest and oldest lines, to save resources

We used to run PCT on the newest and oldest line. Due to the way the lines.txt file is sorted, we now run on the newest LTS line and bom-weekly (which is the last line in the file, after the oldest LTS line). We do not run on any older LTS lines. This does not seem to be intentional. I think at minimum the comment in the Jenkinsfile should be updated, but we may want to consider adding a third set of tests for the oldest LTS line (if that is not prohibitively expensive from a cost perspective).

@jglick jglick mentioned this pull request Mar 17, 2022
Merged
@jglick jglick mentioned this pull request May 19, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olblak olblak olblak left review comments

@jetersen jetersen jetersen left review comments

@jglick jglick jglick approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
5 participants
@timja @jglick @basil @jetersen @olblak