search

anancarv / python-artifactory

Typed interactions with the Jfrog Artifactory REST API
MIT License
55 stars 50 forks source link

Support Artifactory REST APIv2 #88

Closed thenewnano closed 3 years ago

thenewnano commented 3 years ago

The V2 REST APIs apply for Artifactory version 6.6.0 and above, the user can pass the version explicitly.

Currently this only affects Permissions.

Description

The permission scheme has changed in new version of artifactory, I had to find a way to behave based on API version supported by the server. Trying dynamic check of the server version I ended up in 5-6 server calls for each class that was called in class Artifactory.init So At the end I opted for adding an argument that can be send to the constructor at the time the class is instantiated

https://www.jfrog.com/confluence/display/JFROG/Artifactory+REST+API+V2

I turned of the complexity fail on number of arguments, as all the ways around it would add more complexity. but I'm open to suggestions.

Type of change

Please delete options that are not relevant.

How has it been tested ?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Checklist:

thenewnano commented 3 years ago

@anancarv I would appreciate if you or another maintainer could review my PR, as I plan to put this lib in production this week it would be great if we could put together a release.
I have put roughly 8 hours into this including the setup and test etc and like the level of quality you have set up, me and my team will probably contribute more to the project the coming weeks.

I would really want to avoid forking and publishing, my version though, which I'll have to do in a few days.

anancarv commented 3 years ago

Hi @thenewnano , thanks for your contribution. We'll review your PR as soon as possible. I understand that you have production needs, we'll do our best to give you a feedback soon

anancarv commented 3 years ago

I've seen it afterwards, but you have a little typo in the README:

when you instantiate the calss.
thenewnano commented 3 years ago

@anancarv thank you for very vast and constructive comments, I have implemented the changes it should be all good to be merged now! I will probably , maybe before the end of this week, implement the checksum feature that is in the issue list as well.

thenewnano commented 3 years ago

@nymous it should be all good now! Thank you for the great review and the suggestions.

anancarv commented 3 years ago

@thenewnano @sciyako , as said before by @nymous the publication of the PermissionV2 feature may cause issues with the builds. Indeed, if you update an existing permission which has both "repo" and "build" but don't set one or the other, the corresponding sub-permission is removed. Do you want us to publish it as it is and then later work on the builds part ? Or, do you want us to first implement the builds before publishing the new version of the library. We're asking because you seemed to want to feature ASAP.

thenewnano commented 3 years ago

Thank you @nymous and @anancarv this has been fun and inspiring first contribution. Currently I don't have permission to manage builds on the new server I am intending to develop my code against.

So if you can start with a branch that I can help with it would be nice too, if you have not been able to start before I have access to a server to test the build implementation with then I might start.

It would be cool if you do a release to pypi so I can start using this module instead of the one we use today at least where the feature are already available. There are some other features that I might add needed in the project we do now, features needed there will have priority for my contribution to add here instead of the internal closed source module.

anancarv commented 3 years ago

Ok then, a new release has been published v1.9.0. You can now use the permissions v2 feature.