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-58147] Update yaml support to include groupId for incrementals #46

Merged
merged 9 commits into from Jul 25, 2019

Conversation

@stopalopa
Copy link
Contributor

commented Jul 24, 2019

Adds support for using groupId and versions to specify plugins that should be downloaded from the incrementals repository. If a user enters a groupId, tool will assume they want to download plugins from incremental Id. In other words, they can't enter a groupId and try to download from a different url or update center. Maybe this is too rigid? Also updated the REAME yaml example to include real plugin names since Joseph thought what was there before was confusing.

Original PR for adding yaml support: #30

Jenkins Jira:
https://issues.jenkins-ci.org/browse/JENKINS-58147

@stopalopa stopalopa requested a review from jenkinsci/gsoc2019-plugin-installation-manager Jul 24, 2019

README.md Outdated
For plugins listed in a .txt file, each plugin must be listed on a new line. Comments beginning with `#` will be filtered out.

Support for downloading plugins from maven is not yet supported, but is a feature that is planned to be added in the future [JENKINS-58217](https://issues.jenkins-ci.org/browse/JENKINS-58217)

This comment has been minimized.

Copy link
@timja

timja Jul 24, 2019

Member

not sure if this line is needed, have users been asking for it?
seems like url covers most needs if they aren't in an update centre

This comment has been minimized.

Copy link
@stopalopa

stopalopa Jul 24, 2019

Author Contributor

It was brought up once on the channel and once during the last GSoC evaluation presentation.

} else {
Object versionObject = pluginSource.get("version");
if (!StringUtils.isEmpty(groupId) && versionObject == null) {
System.out.println("Version must be input for " + name + ". Skipping...");
continue;

This comment has been minimized.

Copy link
@timja

timja Jul 24, 2019

Member

Should this throw an error? I would like to know if my config is wrong and I'm missing a plugin

source:
version: "incrementals;org.jenkins-ci.plugins.workflow;2.19-rc289.d09828a05a74"
version: 2.19-rc289.d09828a05a74

This comment has been minimized.

Copy link
@timja

timja Jul 24, 2019

Member

🎉

@@ -241,6 +242,7 @@ public void getPluginDownloadUrlTest() {
VersionNumber otherVersion = new VersionNumber("otherversion");

plugin.setVersion(otherVersion);
plugin.setGroupId("");

This comment has been minimized.

Copy link
@timja

timja Jul 24, 2019

Member

is this needed?

This comment has been minimized.

Copy link
@stopalopa

stopalopa Jul 24, 2019

Author Contributor

Yes, the way I currently have it set up. I'm reusing the same plugin to test, but changing it's attributes. Because I'm checking the groupId to determine if a requested plugin should be downloaded from the incrementals repository, this needs to be set back to null or an empty string if the plugin should not be downloaded from an incrementals repository.

This comment has been minimized.

Copy link
@stopalopa

This comment has been minimized.

Copy link
@timja

timja Jul 25, 2019

Member

probably better to create a new plugin instance, it would be clearer, and you can set all the args in the constructor?

perhaps also better to create a separate test case

This comment has been minimized.

Copy link
@stopalopa

stopalopa Jul 25, 2019

Author Contributor

Fixed in 48098fd.

@timja

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Question on the yaml feature,
Do we need the --plugin-yaml I would have expected to just the --plugin-file and detect yaml based on yaml or yml file extension

@stopalopa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Question on the yaml feature,
Do we need the --plugin-yaml I would have expected to just the --plugin-file and detect yaml based on yaml or yml file extension

I thought about this, but wasn't sure if there'd ever be cases where the user would want to want to download plugins from both a .txt file and yaml file....maybe that wouldn't be very common though.

stopalopa added 4 commits Jul 24, 2019
@timja

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Question on the yaml feature,
Do we need the --plugin-yaml I would have expected to just the --plugin-file and detect yaml based on yaml or yml file extension

I thought about this, but wasn't sure if there'd ever be cases where the user would want to want to download plugins from both a .txt file and yaml file....maybe that wouldn't be very common though.

YAGNI, I would make it simpler and not have the extra flag

@timja
Copy link
Member

left a comment

Error handling looks much better, please fix the package for the new class you introduced though

Fix package names
Co-Authored-By: Tim Jacomb <timjacomb1+github@gmail.com>
@timja
timja approved these changes Jul 25, 2019
Copy link
Member

left a comment

👍

@stopalopa stopalopa merged commit b08cdfe into jenkinsci:master Jul 25, 2019

1 check passed

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

@stopalopa stopalopa deleted the stopalopa:yaml branch Jul 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.