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

update pipeline-stage-view hook to adapt the use of incrementals in new versions #232

Merged
merged 3 commits into from
Mar 5, 2020

Conversation

bmunozm
Copy link

@bmunozm bmunozm commented Mar 3, 2020

This updates the pipeline-stage-view to adapt the Hook to make PCT works with version 2.13 od the plugin

This Hook is also compatible with lower versions cc/ @olamy @dwnusbaum @oleg-nenashev @raul-arabaolaza

This PR supersede #231 unless it is adapted properly (in that case this PR can be closed). I tried to update the PR but I am not having permissions to do it, that's the reason why I have to create a new one.

@@ -37,6 +37,6 @@ public static boolean isPipelineStageViewPlugin(Map<String, Object> moreInfo) {
}

public static boolean isPipelineStageViewPlugin(PomData data) {
return data.artifactId.contains("pipeline-rest-api");
return data.groupId.equals("org.jenkins-ci.plugins.pipeline-stage-view") || data.artifactId.contains("pipeline-rest-api");
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd either add another case for data.artifactId.contains("pipeline-stage-view") or drop the case for data.artifactId.contains("pipeline-rest-api") for clarity. I think the groupId check by itself is good enough.

Copy link
Author

Choose a reason for hiding this comment

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

The groupId works for version 2.13 but not for previous one, so the idea is to leave current behaviour until 2.12 and this versions and future one use the new logic in the hook

@@ -23,7 +23,7 @@ protected String getParentProjectName() {

@Override
protected String getPluginFolderName(UpdateSite.Plugin currentPlugin){
return "rest-api";
return (currentPlugin.getDisplayName() == "pipeline-rest-api") ? "rest-api" : "ui";
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want .getDisplayName here instead of .name? I think the display name is something like "Pipeline Stage View", whereas .name is the artifactId, but I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, also you shouldn't compare Strings using ==, you should use .equals(). I think the check is always false right now.

Copy link
Author

Choose a reason for hiding this comment

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

Changed!! I was reviewing and the proper way to get the name is using the getDisplayName(), it is returned properly like pipeline-rest-api

Copy link
Contributor

@raul-arabaolaza raul-arabaolaza left a comment

Choose a reason for hiding this comment

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

LGTM

@bmunozm bmunozm requested a review from dwnusbaum March 3, 2020 15:44
Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

you didn't pick correctly changes from #231 .. Java source file PipelineRestApiHook rename to PipelineStageViewHook but you didn't change the class name in the java source file so the pr do not compile

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

ok I updated the pr

@olamy olamy merged commit a0be34d into jenkinsci:master Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants