Skip to content

Conversation

@Ido-Zerah
Copy link

@Ido-Zerah Ido-Zerah commented Aug 31, 2025

Description

as of now the artifactory api included different routes, som atart with artifactory/api/... however some of the api routes use the other services other that the api.
example:
access, xray, topology and such.
in order to add features that use the other routes apis i added the art-service parameter. this will appended between the base url and the route variable.

in addition in order for the pre-commit-hooks to work i needed to add a dependency to the bandit precommit

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How has it been tested ?

  • Test A- i ran all the existing tests who all passed
  • Test B- I work on a closed off network at my job, in order to add a feature we had to change this functionality, and it works wonderfully

Checklist:

  • My PR is ready for prime time! Otherwise use the "Draft PR" feature
  • All commits have a correct title
  • Readme has been updated
  • Quality tests are green (see Codacy)
  • Automated tests are green (see pipeline)

@anancarv
Copy link
Owner

anancarv commented Sep 5, 2025

Hey @Ido-Zerah ,
Thanks for your PR.
However, I am afraid that this change is not needed as this lib does not support xray, topology ...
When it comes to access, we already support it as you can see it here. If we were to support the other services you mentioned, we would probably do it the same way we did for access.

@Ido-Zerah
Copy link
Author

hi thank you for the reply,
regarding the support for using access.
when it seems that for the other artifactory objects to work i need to initialize an artifactory object with the url ending with /artifactory. however this needs to be trimed if using access and or other endpoints.
am i intended to initialize another object for using access?

@Ido-Zerah
Copy link
Author

Ido-Zerah commented Sep 6, 2025

additionally regarding the tests failing this appears to be because of codacy and not the code actually failing tests.

@eaaltonen
Copy link

When it comes to access, we already support it as you can see it here. If we were to support the other services you mentioned, we would probably do it the same way we did for access.

JFrog has actually moved the Access API from ${BASEURL}/artifactory/access to ${BASEURL}/access. According to https://jfrog.com/help/r/jfrog-rest-apis/introduction-to-the-artifactory-rest-apis this change was done in version 7.49.3. This was actually next on my TODO list also.

@anancarv: Regarding the version compatibility, our choices seem to be:

  • README entry to tell people to use older version with Artifactory versions < 7.49.3
  • some transparent server version based support

I think I'd prefer the former just to keep things simple.

@anancarv
Copy link
Owner

anancarv commented Sep 10, 2025

When it comes to access, we already support it as you can see it here. If we were to support the other services you mentioned, we would probably do it the same way we did for access.

JFrog has actually moved the Access API from ${BASEURL}/artifactory/access to ${BASEURL}/access. According to https://jfrog.com/help/r/jfrog-rest-apis/introduction-to-the-artifactory-rest-apis this change was done in version 7.49.3. This was actually next on my TODO list also.

@anancarv: Regarding the version compatibility, our choices seem to be:

  • README entry to tell people to use older version with Artifactory versions < 7.49.3
  • some transparent server version based support

I think I'd prefer the former just to keep things simple.

Ahh ok, I didn't notice that change in the Access API. I also prefer the first option as it is easier. But, sooner or later, we would need to tackle this issue because we cannot prevent people from upgrading their Artifactory version. So let's fix it.

I would suggest doing:

  • During the artifactory object init, call the endpoint GET /artifactory/api/system/version to fetch the Artifactory version.
  • Add the artifactory version in the Artifactory object constructor
  • Before calling the access endpoint, check whether artifactory_version >= 7.49.3. If yes, then remove artifactory/ from the URL, else do nothing

What do you think?

) # to support base urls with or without /artifactory suffix
response: Response = http_method(
f"{self._artifactory.url}/{route}",
f"{uri}/{art_service}/{route}",

Choose a reason for hiding this comment

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

This doesn't yet fix the issue. Actually with the current commit if I create an Artifactory(url='http://localhost:6543/' and check the behaviour, now create_access_token requests are also directed to the path /artifactory/access/api. It would be better to have a commit or commits that direct access requests to the correct path.

@eaaltonen
Copy link

I would suggest doing:

  • During the artifactory object init, call the endpoint GET /artifactory/api/system/version to fetch the Artifactory version.
  • Add the artifactory version in the Artifactory object constructor
  • Before calling the access endpoint, check whether artifactory_version >= 7.49.3. If yes, then remove artifactory/ from the URL, else do nothing

What do you think?

In principle this would be the least disruptive way of adding client transparent support for older versions.

In the code, create_access_token uses _post and _generic_http_method_request from object.py. Having to do a check for /access/ endpoint is kind of ugly though - which is why I'm wondering do we really need transparent support.

@anancarv
Copy link
Owner

I would suggest doing:

  • During the artifactory object init, call the endpoint GET /artifactory/api/system/version to fetch the Artifactory version.
  • Add the artifactory version in the Artifactory object constructor
  • Before calling the access endpoint, check whether artifactory_version >= 7.49.3. If yes, then remove artifactory/ from the URL, else do nothing

What do you think?

In principle this would be the least disruptive way of adding client transparent support for older versions.

In the code, create_access_token uses _post and _generic_http_method_request from object.py. Having to do a check for /access/ endpoint is kind of ugly though - which is why I'm wondering do we really need transparent support.

Yes you're right. Let's not implement transparent report yet then, and update the README as you suggested.

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.

3 participants