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-51363] Pipeline support #5

Merged
merged 5 commits into from May 28, 2018
Merged

Conversation

cizezsy
Copy link
Contributor

@cizezsy cizezsy commented May 25, 2018

  • add @symbol on CoveragePublisher
  • remove optional params in @DataBoundConstructor constructor, and using @DataBoundSetter public propery setter to config them.
  • fix bug in XMLCoverageReportAdapter, which when plugin is packaged, xsl will not be properly find.

- add @symbol on CoveragePublisher
- remove optional params in @DataBoundConstructor constructor, and using @DataBoundSetter public propery setter to config them.
- fix bug in XMLCoverageReportAdapter, which when plugin is packaged, xsl will not be propery find.
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.

IIUC @Symbols should be added to the converter implementations as well so that the code is simple.

It would be nice if you could also provide an example of how the code look like, preferable in a simple unit test

this.autoDetectPath = autoDetectPath;
}

@Symbol("coverage")
Copy link
Member

Choose a reason for hiding this comment

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

'publishCoverage'?

@cizezsy
Copy link
Contributor Author

cizezsy commented May 27, 2018

@oleg-nenashev Thresh branch is based on this branch, so I think I should not do too many changes in this branch. I will add more unit test in Thresh branch.

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.

Just some wording comments. Everything looks good though the Test builder is an overkill IMHO. But no need to revert

@@ -107,6 +107,7 @@ public CoverageResult processCoverageReport() throws IOException, InterruptedExc

// If enable automatically detecting, it will try to find report and correspond adapter.
if (isEnableAutoDetect()) {
listener.getLogger().println("Auto Detect is enable: Start to detect report");
Copy link
Member

Choose a reason for hiding this comment

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

  1. enabled.
  2. Looking for reports (maybe)


}

public static class CoverageScriptedPipelineScriptBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it was an overkill. It could be preferable to just have scripts, which can be later used for documentation purposes


private String generateSnippetForThreshold(Threshold threshold) {

return "[healthyThresh:" + threshold.getHealthyThresh()
Copy link
Member

Choose a reason for hiding this comment

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

"thresh" is probably a bad wording for UI and API. It has different meaning (@jeffpearce please correct me if I am wrong). Does it come from Cobertura plugin? If no, I would recommend using a full "threashold" everywhere in API and Symbols

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, I will change them in threshold branch.

@@ -35,6 +36,7 @@ public String getXSD() {
return null;
}

@Symbol("coberturaAdapter")
Copy link
Member

Choose a reason for hiding this comment

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

maybe just cobertura() ? "Adapter" is an implementation detail

@@ -26,6 +27,7 @@ public String getXSD() {
return null;
}

@Symbol("jacocoAdapter")
Copy link
Member

Choose a reason for hiding this comment

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

jacoco?

@cizezsy cizezsy merged commit c722e1f into jenkinsci:pipeline May 28, 2018
@cizezsy
Copy link
Contributor Author

cizezsy commented May 28, 2018

I have merged this pull request, and I will do all further changes in thresh branch.

@cizezsy cizezsy deleted the pipeline branch May 28, 2018 06:28
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.

None yet

2 participants