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

[JENKINS-60393] Ensure all plugins are loaded to configure the defaults #136

Merged
merged 7 commits into from Dec 16, 2019

Conversation

Dohbedoh
Copy link
Contributor

@Dohbedoh Dohbedoh commented Dec 12, 2019

See JENKINS-60393: the configuration of the defaults in the constructor of the GlobalConfiguration may cause the loading to fail on startup in some cases

Proposed changelog entries

  • Ensure all plugin are loaded before setting global defaults

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change).
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests

@fcojfernandez @rsandell

@varyvol
Copy link

varyvol commented Dec 12, 2019

The change looks good, but given this comes from a bug I think it's worth to have an automated test.

@fcojfernandez
Copy link
Contributor

fcojfernandez commented Dec 12, 2019

@Dohbedoh I've been doing some testing and I've found weird this:

  1. Install from scratch
  2. Go to Manage Jenkins > Configure and remove all the Health Metrics
  3. Save
  4. Enter again. The Health Metrics are there again

Shouldn't they be removed?

/**
* Auto-configure the default metrics afetr all plugins have been loaded.
*/
@Initializer(after = InitMilestone.EXTENSIONS_AUGMENTED, before = InitMilestone.JOB_LOADED)
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember the implications of having both after and before rules in the same initializer. But I guess you've verified it works as intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested this yes. I am not sure how to create a test for the initialization though ?
The real fix is to perform the auto configuration outside the constructor and when plugins are loaded: after = InitMilestone.EXTENSIONS_AUGMENTED. I do believe that this is enough. I added the before = InitMilestone.JOB_LOADED because I feel this is consistent (the global metrics will be used on Folder's creation)

Co-Authored-By: Robert Sandell <rsandell@cloudbees.com>
@Dohbedoh
Copy link
Contributor Author

@fcojfernandez You are right. Apparently when you remove all metrics, no healthMetrics attributes is included in the JSON at all. And the bind just does nothing. I need to override the GlobalConfiguration#configure method.

@Dohbedoh
Copy link
Contributor Author

@varyvol @rsandell @fcojfernandez any idea how I can write a test for the component initialization problem ?

@varyvol
Copy link

varyvol commented Dec 13, 2019

@Dohbedoh I think if you tried to create a folder, then something was logged? I'd say that's what you need to automate.

} else {
this.setHealthMetrics(Collections.emptyList());
}
Copy link
Member

Choose a reason for hiding this comment

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

Missed save() ?

Maybe simpler to

this.setHealthMetrics(Collections.emptyList());
req.bindJSON(this, json);
this.save();

?

Copy link
Member

Choose a reason for hiding this comment

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

ah I see now the call to save() in setHealthMetrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this would save() twice

@fcojfernandez
Copy link
Contributor

Manual testing seems to be OK. @varyvol 's comment is the only one missing:

@Dohbedoh I think if you tried to create a folder, then something was logged? I'd say that's what you need to automate.

@Dohbedoh
Copy link
Contributor Author

We would need to cause the circular dependency first though. It cause the component to not be loaded (AbstractFolderConfiguration#get thows an IllegalStateException). So maybe I can create an anonymous class with an @Initializer to cause this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants