Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Add initial package-config generation #28

Merged
merged 10 commits into from
Jun 4, 2020
Merged

Add initial package-config generation #28

merged 10 commits into from
Jun 4, 2020

Conversation

sladyn98
Copy link
Contributor

@sladyn98 sladyn98 commented Jun 1, 2020

This pull request adds package-config generation. We would like to pass the form data that we get from the front end in the form of a JSON Object rather than put in tons of arguments therefore the argument that generatePackageConfig takes is a JSONObject and from that object we would construct the yaml file.

  • Added tests

  • Run the build locally

@sladyn98 sladyn98 added enhancement New feature or request backend Related to the backend service labels Jun 1, 2020
@sladyn98 sladyn98 added this to the [GSoC Phase 1] milestone Jun 1, 2020
@sladyn98 sladyn98 requested a review from a team as a code owner June 1, 2020 18:16
@sladyn98 sladyn98 self-assigned this Jun 1, 2020
@sladyn98
Copy link
Contributor Author

sladyn98 commented Jun 1, 2020

Just some feedback if I am in the right direction would be great

kwhetstone
kwhetstone reviewed Jun 2, 2020
View reviewed changes
Copy link
Contributor

@kwhetstone kwhetstone left a comment

Choose a reason for hiding this comment

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

Added some comments. I think first address the coding hints, and then modify the test a bit to check the generated contents.

pom.xml Outdated Show resolved Hide resolved
src/main/java/com/org/jenkins/Custom/Jenkins/Distribution/Service/Util/Util.java Outdated
@@ -0,0 +1,19 @@
package com.org.jenkins.Custom.Jenkins.Distribution.Service.Util;
Copy link
Contributor

Choose a reason for hiding this comment

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

Like the code quality check says!

...m/org/jenkins/Custom/Jenkins/Distribution/Service/generators/PackageConfigGeneratorTest.java Outdated Show resolved Hide resolved
...a/com/org/jenkins/Custom/Jenkins/Distribution/Service/generators/PackageConfigGenerator.java Outdated Show resolved Hide resolved
@sladyn98
Copy link
Contributor Author

sladyn98 commented Jun 2, 2020

I read in one of the blogs that it is not a good idea to test private methods, so IMO we should just write more tests via the public method.

@sladyn98 sladyn98 requested a review from kwhetstone June 3, 2020 07:25
@sladyn98
Copy link
Contributor Author

sladyn98 commented Jun 3, 2020

@kwhetstone This pull request looks good to go.

@sladyn98 sladyn98 mentioned this pull request Jun 3, 2020
Merged
@sladyn98 sladyn98 changed the title (WIP) Add initial package-config generation Add initial package-config generation Jun 3, 2020
LinuxSuRen
LinuxSuRen suggested changes Jun 3, 2020
View reviewed changes
Copy link
Member

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

I'v made some comments.

There's one more thing that might not be perfect. The updateCenter.json file is a little bit big, it will increase the git repository. Importantly updateCenter.json can be stale after a few days. It always being updated.

pom.xml Outdated
<artifactId>junit-jupiter-api</artifactId>
<version>5.6.2</version>
<scope>test</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Please format the indent here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already indented.Is there any different way to indent ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The indent for this dependency is different than the others. I think your editor might be able to auto-fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I just noticed it shows up fine in my IDE.I will have a look

src/test/resources/packagerConfig/emptyDockerTag.json Outdated Show resolved Hide resolved
src/test/resources/packagerConfig/simpleConfig.json Outdated Show resolved Hide resolved
src/test/resources/packagerConfig/emptyCascSection.json Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@sladyn98
Copy link
Contributor Author

sladyn98 commented Jun 3, 2020

There's one more thing that might not be perfect. The updateCenter.json file is a little bit big, it will increase the git repository. Importantly updateCenter.json can be stale after a few days. It always being updated.

This PR deals with adding the downloader #31. I guess for running the tests we should just put a short version of the update center

@sladyn98 sladyn98 requested a review from LinuxSuRen June 3, 2020 19:32
kwhetstone
kwhetstone suggested changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@kwhetstone kwhetstone left a comment

Choose a reason for hiding this comment

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

As a compromise for this PR: could you make update_center.json smaller? Just make your own with a list of 3 plugins with a comment that it's a placeholder. I agree that you shouldn't include that file in the repo, especially since it's getting replaced.

On the positive side, looks like the builds are running

pom.xml Outdated
<artifactId>junit-jupiter-api</artifactId>
<version>5.6.2</version>
<scope>test</scope>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

The indent for this dependency is different than the others. I think your editor might be able to auto-fix this.

@kwhetstone
Copy link
Contributor

Looks like it's not iding test files because the test's aren't running: script returned exit code 143 I think that can addressed in a separate PR

@sladyn98 sladyn98 requested a review from kwhetstone June 4, 2020 07:29
@sladyn98 sladyn98 force-pushed the 13_package_config_generation branch from 922fb55 to 13b735e Compare June 4, 2020 12:36
@sladyn98 sladyn98 merged commit 7a36941 into jenkinsci:master Jun 4, 2020
@sladyn98 sladyn98 mentioned this pull request Jun 5, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kwhetstone kwhetstone kwhetstone approved these changes

@oleg-nenashev oleg-nenashev Awaiting requested review from oleg-nenashev

@LinuxSuRen LinuxSuRen Awaiting requested review from LinuxSuRen

Assignees

@sladyn98 sladyn98

Labels
backend Related to the backend service enhancement New feature or request
Projects
None yet
Milestone
[GSoC Phase 1]
Development

Successfully merging this pull request may close these issues.
3 participants
@sladyn98 @kwhetstone @LinuxSuRen