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

Support API V2 : Get / Create / Delete Permission targets - Issue #256 #383

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

smoneuse
Copy link

@smoneuse smoneuse commented Oct 9, 2023

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • This pull request is on the dev branch.

Hello team !

This PR provides support for API V2 Permission target management : GET / CREATE_OR_UPDATE / DELETE
It respects code actual architecture and added features are tested.

Thanks !

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@smoneuse
Copy link
Author

smoneuse commented Oct 9, 2023

I have read the CLA Document and I hereby sign the CLA

@smoneuse
Copy link
Author

smoneuse commented Oct 9, 2023

recheck

@yahavi yahavi added new feature Automatically generated release notes safe to test Approve running integration tests on a pull request labels Oct 9, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 9, 2023
@axel3rd
Copy link
Contributor

axel3rd commented Oct 31, 2023

Nice contribution 👍 ! Would be nice if could be added !

@axel3rd
Copy link
Contributor

axel3rd commented Nov 14, 2023

@yahavi Is it possible to have an ETA (future 2.x version linked) on this pull-request 😊 ? Or if any update should be done ?

@eyalbe4 eyalbe4 self-requested a review June 27, 2024 10:46
Copy link
Contributor

@eyalbe4 eyalbe4 left a comment

Choose a reason for hiding this comment

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

Let's also add the new APIs to the README.

PermissionTargetV2Builder build(PermissionV2 build);
PermissionTargetV2Builder releaseBundle(PermissionV2 releaseBundle);
PermissionTargetV2 build();

Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant new-line.

if(permissionTarget == null || StringUtils.isBlank(permissionTarget.getName())){
throw new IllegalArgumentException("Permission target name is required")
}
if (permissionTarget.getRepo() == null || permissionTarget.getRepo().getRepositories()==null || permissionTarget.getRepo().getRepositories().isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing white-spaces before null

permissionTarget.getRepo().getRepositories()==null

if (permissionTarget.getRepo() == null || permissionTarget.getRepo().getRepositories()==null || permissionTarget.getRepo().getRepositories().isEmpty()) {
throw new UnsupportedOperationException("At least 1 repository is required in permission target (could be 'ANY', 'ANY LOCAL', 'ANY REMOTE')")
}
if(permissionTarget.getBuild() != null && permissionTarget.getBuild().getRepositories() !=null){
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing white-space after if

if(permissionTarget.getBuild()

if (permissionTarget.getRepo() == null || permissionTarget.getRepo().getRepositories()==null || permissionTarget.getRepo().getRepositories().isEmpty()) {
throw new UnsupportedOperationException("At least 1 repository is required in permission target (could be 'ANY', 'ANY LOCAL', 'ANY REMOTE')")
}
if(permissionTarget.getBuild() != null && permissionTarget.getBuild().getRepositories() !=null){
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing white-space before {

getBuild().getRepositories() !=null){

}
if(permissionTarget.getBuild() != null && permissionTarget.getBuild().getRepositories() !=null){
List<String> buildRepositories = permissionTarget.getBuild().getRepositories();
if(buildRepositories.size() !=1 || !buildRepositories.contains("artifactory-build-info")){
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing white-spaces in this line.

import org.jfrog.artifactory.client.model.PermissionV2;

import java.util.Optional;

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new-lines according to the code conventions in a few places in this file.


public class PermissionV2BuilderImpl implements PermissionV2Builder {


Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant new-line.

@@ -191,6 +191,44 @@ protected String deleteRepoIfExists(String repoName) {
}
}
}
protected void deletePermissionTargetV2IfExists(String name){
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new-line between methods.

final String GROUP_TEST_2="group_test_2_"+System.currentTimeMillis();
final String PERMISSION_TARGET_NAME="permission_target_v2_"+System.currentTimeMillis();
try{
//Eventual clean up of previous test run
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing white-space after //
(The same appliues for the other comments in this file).

final String GROUP_TEST="group_test_"+System.currentTimeMillis();
final String PERMISSION_TARGET_NAME="permission_target_v2_"+System.currentTimeMillis();
try{
// remove eventual previous test run leftovers
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have all comments start with an upper-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Automatically generated release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants