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-47634] Move metadata about split plugins into a resource file #3110

Merged
merged 3 commits into from Oct 28, 2017

Conversation

3 participants
@jglick
Member

jglick commented Oct 25, 2017

So that plugin-compat-tester need not be manually synchronized.

See JENKINS-47634.

Proposed changelog entries

  • Metadata about plugins split from core is now accessible from plugin-compat-tester, simplifying some test scenarios.

@reviewbybees @jenkinsci/code-reviewers

[JENKINS-47634] Move metadata about split plugins into a resource fil…
…e so that plugin-compat-tester need not be manually synchronized.
@reviewbybees

This comment has been minimized.

reviewbybees commented Oct 26, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev

Generally the change looks good to me, but it would be great to improve it a bit (see the comments). WDYT @jglick ?

static {
List<DetachedPlugin> detached = new ArrayList<>();
try (InputStream is = ClassicPluginStrategy.class.getResourceAsStream("/jenkins/split-plugins.txt")) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 26, 2017

Member

It would be great to handle null resources, just in case the resource is missing due to whatever reason (repackaged WAR, classloader issues, etc.). Currently the code will fail with NPE

This comment has been minimized.

@jglick

jglick Oct 26, 2017

Member

That is fine with me. If someone deletes resources from jenkins-core.jar, they will get an error.

throw new ExceptionInInitializerError(x);
}
DETACHED_LIST = ImmutableList.copyOf(detached);
try (InputStream is = ClassicPluginStrategy.class.getResourceAsStream("/jenkins/split-plugin-cycles.txt")) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 26, 2017

Member

same as above: NPE handling and comment support

List<DetachedPlugin> detached = new ArrayList<>();
try (InputStream is = ClassicPluginStrategy.class.getResourceAsStream("/jenkins/split-plugins.txt")) {
for (String line : org.apache.commons.io.IOUtils.readLines(is, StandardCharsets.UTF_8)) {
String[] pieces = line.split(" ");

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 26, 2017

Member

I would propose to skip lines starting from # and then to document the format directly in the resource files. It would help future contributors

This comment has been minimized.

@jglick

jglick Oct 26, 2017

Member

Possible. Would need to make a matching change in the downstream PR, and then restart the manual test cycle.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 26, 2017

Member

Comments would be really useful in order to reference plugin split tickets and other info. I am fine if we create a follow-up JIRA ticket and proceed with this change without such patches

This comment has been minimized.

@jglick

jglick Oct 26, 2017

Member

I am not sure it could be done safely as a follow-up: again, the downstream PR needs to match the format.

Thirty seconds to write the code for such a change, an hour to recreate the test environment needed to verify it. I will see if I can find time for it, but I do not know.

"credentials/matrix-auth",
"credentials/windows-slaves"
));
private static final Set<String> BREAK_CYCLES;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 26, 2017

Member

Maybe it makes sense to convert the field to a more intelligent data structure in order to avoid parsing fields each time.

This comment has been minimized.

@jglick

jglick Oct 26, 2017

Member

We do not actually parse the elements of this set—we just test for membership.

@oleg-nenashev

🐝

@jglick

This comment has been minimized.

Member

jglick commented Oct 27, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 27, 2017

I would add a Developer entry to the changelog, the files may be useful in other cases (proof?).

@jenkinsci/code-reviewers any concerns regarding merging it towards the next weekly?

@jglick jglick merged commit 97e6e40 into jenkinsci:master Oct 28, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@jglick jglick deleted the jglick:metadata-JENKINS-47634 branch Oct 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment