Conversation
dwnusbaum
left a comment
dwnusbaum
left a comment
There was a problem hiding this comment.
Took a quick look and added some general feedback about things that I noticed. Nice work!
| <build> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <configuration> | ||
| <source>16</source> | ||
| <target>16</target> | ||
| </configuration> | ||
| </plugin> | ||
| </plugins> | ||
| </build> |
There was a problem hiding this comment.
Don't do this. Let the parent POM control the Java version, see https://github.com/jenkinsci/plugin-pom/blob/03cc96f91d5ddfa4b46acfc78267baf8895472d1/pom.xml#L61.
There was a problem hiding this comment.
I think that this came as part of the jenkins plugin template and has not noticed.
Thanks for the observation and I'll update it to inherit it from the parent.
| <dependency> | ||
| <groupId>com.google.code.gson</groupId> | ||
| <artifactId>gson</artifactId> | ||
| <version>2.11.0</version> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>com.google.errorprone</groupId> | ||
| <artifactId>error_prone_annotations</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> |
There was a problem hiding this comment.
Generally in Jenkins, avoid pulling in 3rd party Java libraries directly wherever possible. Prefer API plugins. See https://www.jenkins.io/doc/developer/plugin-development/dependencies-and-class-loading/#bundling-third-party-libraries and the next two headings. In Jenkins, basic JSON-related tasks can use https://github.com/jenkinsci/jenkins/blob/b0e47a7a9bf5b448bb906e7087cac841b2b5b144/core/pom.xml#L397-L400 (org.net.sf.json.JSONObject is the main class), and for more complicated cases you can use Jackson via https://plugins.jenkins.io/jackson2-api/.
|
|
||
| private String username; | ||
|
|
||
| private String password; |
There was a problem hiding this comment.
Never use a String for secret values that get persisted. Either use the Secret type, or typically better is to have the user configure credentials separately and then just have the user configure the ID of the relevant credentials. See https://www.jenkins.io/doc/developer/security/secrets/.
There was a problem hiding this comment.
yeah, makes sense. I'll update it.
| private static FormValidation invokeRestApiPost(String url, String authHeader, String payload) { | ||
| try { | ||
| HttpClient client = HttpClient.newHttpClient(); | ||
| HttpRequest.Builder requestBuilder = HttpRequest.newBuilder() |
There was a problem hiding this comment.
FTR in Jenkins use these methods to create HttpClient instances to respect users' proxy configurations.
| return url; | ||
| } | ||
|
|
||
| public void setUrl(String url) { |
There was a problem hiding this comment.
FWIW I think it is typical to use @DataBoundSetter for GlobalConfiguration setters and @DataBoundConstructor for the constructor, though perhaps it is not required? See for example https://github.com/jenkinsci/configuration-as-code-plugin/blob/8b9efc1b9026942064d5248c92bf89cb00f83ea0/plugin/src/main/java/io/jenkins/plugins/casc/CasCGlobalConfig.java#L38.
| } | ||
| } | ||
|
|
||
| private void saveJobNamePerCategory(String jobName, String category) { |
There was a problem hiding this comment.
In Jenkins "Job" and "Run" mean two very distinct things, so it is important to be clear about what you mean. This is runFullDisplayName, not jobName.
| Type type = new TypeToken<Map<String,List<JobRunEntry>>>(){}.getType(); | ||
| categoryJobsMap = gson.fromJson(reader, type); |
There was a problem hiding this comment.
Seeing the use case now, in this case probably the typical approach would just be to serialize this as XML instead using XStream from Jenkins core.
| public ListBoxModel doFillSelectedCategoryItems() { | ||
| ListBoxModel items = new ListBoxModel(); | ||
| OnboardingSectionConfiguration onboardingSectionConfig = GlobalConfiguration.all().get(OnboardingSectionConfiguration.class); | ||
| System.out.println(onboardingSectionConfig == null); |
There was a problem hiding this comment.
Make sure not to do this in real code.
| } | ||
| ``` | ||
| 2. Install pipeline-utility-steps | ||
| 3. Approve method invocations from http://localhost:8080/jenkins/manage/scriptApproval/ |
There was a problem hiding this comment.
If you ever have to approve methods on this page you are probably going down the wrong path. getBuildReferences and updateBuildReferences don't look like they belong in a Pipeline at all. I think the expectation was for this logic to be done internally in OnboardingTaskBuilder.
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>structs</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
I don't think you need this dependency.
2 participants
This PR contains solutions of all the tasks mentioned in https://engineering.beescloud.com/docs/engineering-cbci/latest/onboarding/practical-work-plugin.