Support for setting artifact properties

anancarv / python-artifactory

Typed interactions with the Jfrog Artifactory REST API

MIT License

55 stars 50 forks source link

Support for setting artifact properties #92

Closed deyanstoykov closed 2 years ago

deyanstoykov commented 3 years ago

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes #91

Type of change

[x] New feature (non-breaking change which adds functionality)

How has it been tested ?

Tested manually with Artifactory 7:

[x] set/update a single property on an existing artifact
[x] set/update multiple properties on an existing artifact
[x] set/update an empty property list on an existing artifact
[x] set/update a single property on an non-existing artifact (ArtifactNotFoundException is thrown)
[x] set/update a single property on an existing artifact in non-recursive mode (Note: There is a known issue: RTFACT-25982)

As the API calls used do not return any data other than the HTTP status code, I'm not sure what's the proper approach to automated testing.

Checklist:

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

anancarv commented 3 years ago

Hi @deyanstoykov , sorry for the late answer. I'll have a look at it asap

deyanstoykov commented 2 years ago

Can you also add some unit tests for these new features please ? You can write the following tests: set_property_success -> 200 OK set_property_fail_bad_fields -> 400 KO set_property_fail_artifact_not_found -> 404 KO

update_property_success -> 200 OK update_property_fail_bad_fields -> 400 KO update_property_fail_artifact_not_found -> 404 KO

Instead of returning nothing in the set_properties and update_properties functions, you can return a property:
return self.properties(artifact_path, [List of properties])
https://github.com/anancarv/python-artifactory/blob/71f92520da057dd1a4ca66828d5e14871388dfaf/pyartifactory/objects.py#L884

As a result, in your tests you can assert that the properties have been successfully set.

Unit tests added.

deyanstoykov commented 2 years ago

Can you also add some unit tests for these new features please ? You can write the following tests: set_property_success -> 200 OK set_property_fail_bad_fields -> 400 KO set_property_fail_artifact_not_found -> 404 KO

update_property_success -> 200 OK update_property_fail_bad_fields -> 400 KO update_property_fail_artifact_not_found -> 404 KO

Instead of returning nothing in the set_properties and update_properties functions, you can return a property:
return self.properties(artifact_path, [List of properties])
https://github.com/anancarv/python-artifactory/blob/71f92520da057dd1a4ca66828d5e14871388dfaf/pyartifactory/objects.py#L884

As a result, in your tests you can assert that the properties have been successfully set.

Unit tests added.

deyanstoykov commented 2 years ago

Hi @anancarv, @nymous, Looks like all checks have passed. Is there anything I could do to help accelerate approval/merge of this PR?

anancarv commented 2 years ago

Hi @anancarv, @nymous, Looks like all checks have passed. Is there anything I could do to help accelerate approval/merge of this PR?

Hi @deyanstoykov , @nymous wanted to add some comments on this PR. Let's wait for his review. If it's urgent, we can probably merge it and make another PR or create issues to address them. What do you think @nymous ?

anancarv commented 2 years ago

Hi @anancarv, @nymous, Looks like all checks have passed. Is there anything I could do to help accelerate approval/merge of this PR?

The PR has been merged to master. A new release will be created soon