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

Automate Java version recommendation administrative monitor #8526

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

basil
Copy link
Member

@basil basil commented Sep 26, 2023

This PR documents the steps for adding support for a new Java version in JavaVersionRecommendationAdminMonitor and adds one additional step: defining the date the Jenkins project will drop support for that Java version. An administrative monitor is automatically shown two years before that date. If the user dismisses that monitor and declines to upgrade, another (red) "danger" administrative monitor will be shown one year before that date. The ID of the administrative monitor is keyed off the Java version, EOL date, and severity of the warning so that changes to the dates automatically invalidate previous dismissals. Users are directed to https://jenkins.io/redirect/java-support/ for help, which is consistent with the URL used in executable.Main. It is the responsibility of the Documentation Officer to keep this URL fresh with reasonable content.

An alternative design would have been to define a recommended Java version in addition to the set of supported Java versions. The disadvantage of this approach is that more manual steps are required. The advantage is that the result is guaranteed to be accurate, as opposed to the approach in this PR (which could potentially be communicating incorrect information to users if the Jenkins project ever falls dramatically behind in its Java Platform support). We believe that the advantages of automation outweigh the disadvantages. In the unlikely event that the Jenkins project ever falls far behind again in Java Platform support, the approach in this PR could be scaled back in favor of a more manual communication schedule.

Since this change will be delivered after September 30, 2024, it will immediately result in a "danger" (red) warning for all users who are running on Java 11 and have not upgraded to Java 17.

Testing done

  • Ran on Java 17 and 19; verified that no warning was printed.
  • Ran on Java 11 and verified the warning was printed and went away after dismissed, being persisted in the correct ID format.
  • Artificially changed the EOL date to be less than a year away and verified the severity increased to "danger" and the color changed to red.

Proposed changelog entries

  • Automate the display of an administrative monitor when approaching Java end of life (EOL) dates.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

@basil basil added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Sep 26, 2023
*/
@Restricted(DoNotUse.class)
public Date getEndOfLifeAsDate() {
return Date.from(getEndOfLife().atStartOfDay(ZoneId.systemDefault()).toInstant());
Copy link
Member Author

Choose a reason for hiding this comment

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

Jelly localization requires a Date object rather than the (more appropriate) LocalDate. Here we downconvert to support Jelly localization.

Comment on lines +188 to +189
SUCCESS,
INFO,
Copy link
Member

Choose a reason for hiding this comment

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

Unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. Was there a point you were trying to make?

Copy link
Member

@daniel-beck daniel-beck Sep 26, 2023

Choose a reason for hiding this comment

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

Seems like it should be deleted if unused. A random admin monitor doesn't need to be the reference for what CSS classes exist on the UI. If there's a reason this is here while unused, it's nonobvious and should be documented in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well this is a model object, so just like a Hibernate model object where every field has a getter and setter, some are going to be unused. But the model object should be complete so that if someone wants to use the model in a different way then they can do so in the future. Model objects are an exception to the general wisdom around deleting unused code, and (trust me) I love deleting code — the majority of my efforts as a programmer are around code cleanup rather than greenfielding. In any case, I've never added comments to unused getters in e.g. a Hibernate model object and I do not see a compelling reason to do so here.

@NotMyFault NotMyFault requested a review from a team September 28, 2023 10:25
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

nice

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 28, 2023
@MarkEWaite MarkEWaite merged commit 02502e2 into jenkinsci:master Sep 29, 2023
16 checks passed
flabrie pushed a commit to flabrie/jenkins that referenced this pull request Sep 29, 2023
* master:
  Automate Java version recommendation administrative monitor (jenkinsci#8526)
  Avoid saving disabled status when deleting a project (jenkinsci#8528)
  Bump org.jenkins-ci.main:jenkins-test-harness from 2064.vcd3b_b_8f3f2b_a_ to 2085.va_c531db_287b_d (jenkinsci#8535)
  [JENKINS-72067] High memory usage from `XStream2.AssociatedConverterImpl` (jenkinsci#8529)
@MarkEWaite
Copy link
Contributor

MarkEWaite commented Oct 2, 2023

@basil the recommend Java version message has an incorrect string in the /manage/configure page segment that appears when I click the button under the "Administrative monitors configuration" heading. It looks like this:

Screenshot 2023-10-01 215118

I'm not sure if I'm understanding the code correctly, but I thought it would insert the feature release number of the current JVM in that string. I think that we want the next LTS version in that string rather than the Java version that is currently running.

I think that it might be better if the danger period were 9 months and the warning period were 18 months so that we have 6 months between the initial support of a Java version and the visibility of the end of life warning for the upcoming end of life Java version. Alternately, I think danger period of 12 months and warning period of 21 months would be fine as well.

@basil
Copy link
Member Author

basil commented Oct 2, 2023

@MarkEWaite The first issue can be fixed by rewording the message to not use an argument parameter. The second issue can be fixed by modifying the code to use the desired periods.

@MarkEWaite
Copy link
Contributor

@MarkEWaite The first issue can be fixed by rewording the message to not use an argument parameter. The second issue can be fixed by modifying the code to use the desired periods.

Thanks! I've proposed those changes in

Apologies for my delay reviewing the pull request. I had to sketch the calendar dates several times in order to see when the admin monitor warning will appear, when the admin monitor danger will appear and how that matches with the Java LTS release dates and the transition from Java 11 to Java 17 to Java 21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants