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

Reduce log level on notice about optional extensions #4617

Merged
merged 1 commit into from
Apr 11, 2020

Conversation

jglick
Copy link
Member

@jglick jglick commented Apr 1, 2020

Noticed an inconsistency in jenkinsci/kubernetes-credentials-plugin#20. Extends #2305 to better match ec61660 which uses FINE for optional extensions. If an extension is optional and it is not loaded, that is usually because the required plugin is missing, which is not interesting—do not clutter the log with it.

Proposed changelog entries

  • By default suppress log message about a missing optional extension.

Maintainer checklist

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

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a JIRA issue should exist and be labeled as lts-candidate

@daniel-beck daniel-beck self-requested a review April 1, 2020 20:25
@daniel-beck
Copy link
Member

Did we ever resolve the bug which resulted in batch-installs of plugins not considering optional dependencies for install/load order? That would be made more difficult to diagnose with this message gone on regular instances. Which of two plugins implements an extension depending on the plugin is sometimes basically arbitrary, and in a few cases changed over time.

In general, this looks like a message appearing generally during startup, which is pretty cluttered anyway, and then only once per extension, so why not accept a little more log spam to address the cases in which the outcome is unexpected?

@jglick
Copy link
Member Author

jglick commented Apr 1, 2020

Did we ever resolve the bug which resulted in batch-installs of plugins not considering optional dependencies for install/load order?

No clue.

why not accept a little more log spam

To be clear, this patch is bringing this one case (arising from an obscure aspect of Guice behavior) in line with others of longer standing (including direct SezPoz loading). As seen in the linked PR, Jenkins already logged some optional extension failures at FINE, just not consistently. You can always turn on fine logging here if you suspect a Jenkins bug, but the expectation is that not loading an optional extension is quite normal and not worth mentioning.

@res0nance res0nance added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Apr 2, 2020
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

🤷‍♂ Easy enough to revert if necessary.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Fine with me

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 10, 2020
@oleg-nenashev
Copy link
Member

I plan to merge it tomorrow if no negative feedback

@oleg-nenashev oleg-nenashev merged commit e4619eb into jenkinsci:master Apr 11, 2020
@jglick jglick deleted the ExtensionFinder-logging branch April 13, 2020 15:27
This pull request was closed.
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.

6 participants