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.

Sign up for GitHub

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

Jump to bottom

Update hooks to make more plugins works properly with PCT (GitHub, Pipeline: REST API, Jenkins Design Language) #185

Merged
merged 6 commits into from
Sep 11, 2019
Merged

Update hooks to make more plugins works properly with PCT (GitHub, Pipeline: REST API, Jenkins Design Language) #185

merged 6 commits into from
Sep 11, 2019

Conversation

bmunozm
Copy link

@bmunozm bmunozm commented Sep 4, 2019 •
edited
Loading

This PR is about making plugins working with the PCT that needs hooks, special tags and so on.

Fixes included:

  • jenkins-design-language module in blueocean
  • github plugin
  • pipeline-rest-api

@bmunozm bmunozm changed the title Update hooks to make more plugins works properly with PCT [WIP] Update hooks to make more plugins works properly with PCT Sep 4, 2019
@bmunozm bmunozm changed the title [WIP] Update hooks to make more plugins works properly with PCT Update hooks to make more plugins works properly with PCT Sep 10, 2019
oleg-nenashev
oleg-nenashev requested changes Sep 10, 2019
View reviewed changes
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.

BlueOcean filter is too aggressive IMO. it will match all components with BO in the group ID, Could we limit it to a JDL case only?

plugins-compat-tester-model/src/main/java/org/jenkins/tools/test/model/PomData.java
@@ -45,8 +46,9 @@
private String connectionUrl;
private List<String> warningMessages = new ArrayList<String>();

public PomData(String artifactId, @CheckForNull String packaging, String connectionUrl, @CheckForNull MavenCoordinates parent){
public PomData(String artifactId, @CheckForNull String packaging, String connectionUrl, @CheckForNull MavenCoordinates parent, String groupId){
Copy link
Member

Choose a reason for hiding this comment

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

Retain binary compatibility?

Copy link

Choose a reason for hiding this comment

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

I don't think it's used outside? But worth to verify that.

Copy link
Author

Choose a reason for hiding this comment

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

It is only used in this repo, so I guess it is fine.

Copy link
Member

Choose a reason for hiding this comment

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

We can live with that

varyvol
varyvol reviewed Sep 10, 2019
View reviewed changes
plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/PipelineRestApiHook.java Outdated
return isPSVPlugin(info);
}

public static boolean isPSVPlugin(Map<String, Object> moreInfo) {
Copy link

Choose a reason for hiding this comment

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

In my opinion a longer name wouldn't hurt (you need to look at the code to understand which plugin it is).

Copy link
Author

Choose a reason for hiding this comment

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

changed @varyvol

Copy link
Author

Choose a reason for hiding this comment

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

changed!

plugins-compat-tester-model/src/main/java/org/jenkins/tools/test/model/PomData.java
@@ -45,8 +46,9 @@
private String connectionUrl;
private List<String> warningMessages = new ArrayList<String>();

public PomData(String artifactId, @CheckForNull String packaging, String connectionUrl, @CheckForNull MavenCoordinates parent){
public PomData(String artifactId, @CheckForNull String packaging, String connectionUrl, @CheckForNull MavenCoordinates parent, String groupId){
Copy link

Choose a reason for hiding this comment

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

I don't think it's used outside? But worth to verify that.

@raul-arabaolaza
Copy link
Contributor

BlueOcean filter is too aggressive IMO. it will match all components with BO in the group ID, Could we limit it to a JDL case only?

I believe that is already solved, now checks also the artifactId, here we can be less or more agressive, in one case we may run the hook for a wrong plugin, in another we may not run the hook for a needed plugin... Now we are in the second position which I believe is better and we will not break previously working plugins so approving it

oleg-nenashev
oleg-nenashev approved these changes Sep 11, 2019
View reviewed changes
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.

My blocker comment was addressed

@oleg-nenashev oleg-nenashev merged commit 9e0f2f4 into jenkinsci:master Sep 11, 2019
@oleg-nenashev oleg-nenashev changed the title Update hooks to make more plugins works properly with PCT Update hooks to make more plugins works properly with PCT (GitHub, Pipeline: REST API, Jenkins Design Language) Sep 11, 2019
@oleg-nenashev oleg-nenashev mentioned this pull request Sep 11, 2019
Merged
@oleg-nenashev oleg-nenashev mentioned this pull request Oct 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imonteroperez imonteroperez imonteroperez approved these changes

@raul-arabaolaza raul-arabaolaza raul-arabaolaza approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@varyvol varyvol varyvol approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bmunozm @raul-arabaolaza @imonteroperez @oleg-nenashev @varyvol