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

Artifactory Groovy Script conversion #3938

Merged

Conversation

TheMeinerLP
Copy link
Contributor

@TheMeinerLP TheMeinerLP commented May 24, 2024

This PR converts the two last Groovy scripts and reworks the code in readability. The framework Reactor was integrated to allow more speed and parallel work.

Many of the main parts have been split from the Updater class into smaller classes, moving the Updater class into a launcher package to separate start classes from other logic.

Java 21 features such as sealed interfaces are now also used in the core to build a better API.

@TheMeinerLP TheMeinerLP requested a review from a team as a code owner May 24, 2024 18:26
@TheMeinerLP TheMeinerLP marked this pull request as draft June 12, 2024 13:46
@TheMeinerLP TheMeinerLP marked this pull request as draft June 12, 2024 13:46
@TheMeinerLP TheMeinerLP marked this pull request as draft June 12, 2024 13:46
@TheMeinerLP
Copy link
Contributor Author

Waiting for #3936 get merged

@TheMeinerLP TheMeinerLP marked this pull request as ready for review July 24, 2024 12:25
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

looks ok from what I can see, one nit, have you tested this in some way?

timja

This comment was marked as duplicate.

@TheMeinerLP
Copy link
Contributor Author

looks ok from what I can see, one nit, have you tested this in some way?

I would compare the json files of the master and my branch, there should be no differences. I would consider it valid

@timja
Copy link
Member

timja commented Jul 24, 2024

@daniel-beck do you want to check this one?

@daniel-beck
Copy link
Contributor

@timja Thanks for the ping. I remember test coverage wasn't great when I built this (anything?) so how well has this been tested? Any schedule for merge?

@timja
Copy link
Member

timja commented Jul 24, 2024

@timja Thanks for the ping. I remember test coverage wasn't great when I built this (anything?) so how well has this been tested? Any schedule for merge?

No test coverage that I can tell.
No testing done that I'm aware of, JSON files mentioned here aren't really applicable I think as this is about applying permissions: #3938 (comment)

Schedule for merge no not really although this is done as part of GSOC I believe so @NotMyFault or @TheMeinerLP may be able to comment.

@NotMyFault
Copy link
Member

@timja Thanks for the ping. I remember test coverage wasn't great when I built this (anything?) so how well has this been tested? Any schedule for merge?

No test coverage that I can tell. No testing done that I'm aware of, JSON files mentioned here aren't really applicable I think as this is about applying permissions: #3938 (comment)

Schedule for merge no not really although this is done as part of GSOC I believe so @NotMyFault or @TheMeinerLP may be able to comment.

Test coverage does not exist (yet), but in case somethings breaks, we can always revert

@NotMyFault NotMyFault requested a review from timja July 31, 2024 13:12
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

fine from my pov

@NotMyFault NotMyFault merged commit 688b7b8 into jenkins-infra:master Aug 11, 2024
3 checks passed
@TheMeinerLP TheMeinerLP deleted the feature/artifactory-api-conversation branch August 11, 2024 18:48
timja added a commit that referenced this pull request Aug 21, 2024
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