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

Jenkinsfile: longer history, less artifacts #576

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Jenkinsfile: longer history, less artifacts #576

merged 3 commits into from
Dec 16, 2024

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Dec 12, 2024

Keep more builds to get better statistics for master and BETA branches.
But less builds for others (especially PRs).

eclipse-platform/eclipse.platform.releng.aggregator#2653

Keep more builds to get better statistics.
But throw away artifacts after 99 days (~a quarter year). 
This should clean outdated PRs a bit.

eclipse-platform/eclipse.platform.releng.aggregator#2653
@akurtakov
Copy link
Contributor

@fredg02 I would like to hear from you before pushing such changes .

@jukzi
Copy link
Contributor Author

jukzi commented Dec 12, 2024

cc @fredg02

@jukzi
Copy link
Contributor Author

jukzi commented Dec 12, 2024

i took the eclipse.jdt.debug repo first for a try because it has not as much traffic as other repos

Jenkinsfile Outdated
@@ -1,7 +1,7 @@
pipeline {
options {
timeout(time: 40, unit: 'MINUTES')
buildDiscarder(logRotator(numToKeepStr:'15'))
buildDiscarder(logRotator(numToKeepStr: '100', artifactNumToKeepStr: '15', artifactDaysToKeepStr: '99'))
Copy link

Choose a reason for hiding this comment

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

So far, I did not see reliable behavior with artifactDaysToKeepStr, so I'd recommend avoiding it completely.

In this case, the combination of values also does not make much sense. If you only keep up to 15 builds with artifacts, and you build almost daily, artifacts will rarely get 99 days old. As discussed in eclipse-platform/eclipse.platform.releng.aggregator#2653, going with conditional values based on the branch is more robust.

I'd start with:

buildDiscarder(logRotator(numToKeepStr: ('master' == env.BRANCH_NAME) ? '50' : '5', artifactNumToKeepStr: '2')

And in a few weeks you can check back how often you wanted to access the artifacts of builds that are older than 2 builds. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

artifacts will rarely get 99 days old

look in JDT.core PRs, nobody cleans it:
image
the implementation looks solid
https://github.com/jenkinsci/jenkins/blob/5404f14af18155d62f819279a5debddcbff794c5/core/src/main/java/hudson/tasks/LogRotator.java#L213

Copy link
Contributor Author

@jukzi jukzi Dec 12, 2024

Choose a reason for hiding this comment

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

And in a few weeks you can check back how often you wanted to access the artifacts of builds that are older than 2 builds.

I know i did it in the past. Not often, but after almost every vacation because things got screwed up.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is because there are still open PRs for them like eclipse-jdt/eclipse.jdt.core#30 . If/when a PR is merged/closed it will be cleaned up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cancel "will be" and use "would be" - nobody in jdt.core does care.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Hmm. That's a different job though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the plan would be to apply such change to all our builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where is it configured for the I-Build?

Copy link

Choose a reason for hiding this comment

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

@fredg02
Copy link

fredg02 commented Dec 12, 2024

nobody in jdt.core does care.

That's a serious issue and should be addressed. Keeping open PRs around for too long is both frustrating for the author of the PR, but also for committers and release engineers.

@jukzi
Copy link
Contributor Author

jukzi commented Dec 12, 2024

should be addressed.

come on, getting a first PR reviewed after several years (@vogella) is kinda cool. Those guys are just overwhelmed by work.

@vogella
Copy link
Contributor

vogella commented Dec 13, 2024

should be addressed.

come on, getting a first PR reviewed after several years (@vogella) is kinda cool.

Not sure what this refers to but I think this is very harmful behavior and not cool.

@jukzi
Copy link
Contributor Author

jukzi commented Dec 13, 2024

sure, i tried to make a joke. its sad

@jukzi
Copy link
Contributor Author

jukzi commented Dec 13, 2024

@iloveeclipse i also want to hear your opinion about the actual values, as you had increased them.

@jukzi jukzi requested a review from iloveeclipse December 13, 2024 08:09
@jukzi
Copy link
Contributor Author

jukzi commented Dec 16, 2024

merging to this repo to see if it works as intended

@jukzi jukzi merged commit 69a7703 into master Dec 16, 2024
11 checks passed
@jukzi jukzi deleted the jukzi-patch-1 branch December 16, 2024 13:59
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