Delete yum artifacts regex

[f18m]

To be written

[thiagofigueiro]

Yeah that sounds fine. I might write some integration deletion tests to ensure before/after behaves the same. Cheers

Sent on the run; please excuse my brevity.

On 23 Nov 2019, at 22:51, Francesco Montorsi [email protected] wrote:

 @f18m commented on this pull request.

In src/nexuscli/cli/subcommand_repository.py:

@@ -24,6 +23,9 @@ [--blob=<store_name>] [--strict-content] [--cleanup=<c_policy>] [--write=<w_policy>] [--depth=<repo_depth>]

• nexus3 repository (delete|del) <repo_name> [--force]
• nexus3 repository del_assets_regex <repo_name> <assets_regex> [--force] Hi @thiagofigueiro , I'm trying to do the change but I admit I'm facing several issues as I don't know the code so well. For example: the "nexus3 (delete|del) <repository_path>" command is not using the "api/repository/*" code, while my PR has added all the code to perform delete operations in the API first. Should I change the "delete|del" command to go through the API? In practice such existing "delete|del" command can be implemented as a special case of the wildcard code where simply no wildcards are provided...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[f18m]

Yeah that sounds fine. I might write some integration deletion tests to ensure before/after behaves the same.

ok thanks. I managed to run the tests on my system btw so I might try to write new tests as well and adjust the existing one. However I have a bigger trouble: when I add --regex or --wildcard to the delete CLI command, the whole cmd is not recognized anymore as a 'root' command and is instead routed to _run_subcommand() function in the CLI main...not sure why though.. I tried looking at docopt but couldn't find any obvious reason for that...

[f18m]

However I have a bigger trouble: when I add --regex or --wildcard to the delete CLI command, the whole cmd is not recognized anymore as a 'root' command and is instead routed to _run_subcommand() function in the CLI main...not sure why though.. I tried looking at docopt but couldn't find any obvious reason for that...

Ping in case you have some idea to unblock me :) thanks!

[f18m]

However I have a bigger trouble: when I add --regex or --wildcard to the delete CLI command, the whole cmd is not recognized anymore as a 'root' command and is instead routed to _run_subcommand() function in the CLI main...not sure why though.. I tried looking at docopt but couldn't find any obvious reason for that...

Ping in case you have some idea to unblock me :) thanks!

btw after some more attempts to get CLI option parsing working I tried the following:

 git clone https://github.com/thiagofigueiro/nexus3-cli.git
 cd nexus3-cli
 pip3 install -e . ; clear ; nexus3 upload foo bar --norecurse

and I noticed that also this case does not work: I get the docopt help printed instead of the upload command output. If I remove the --norecurse option from the invocation it works!

This look like a bug... it is happening on my system with docopt==0.6.2 (latest)...

[f18m]

Hi @thiagofigueiro , I have integrated the CLI fix done recently in master and with that one I was able to test again all this PR. I think now the MR is in a good shape and ready for re-review.

It's still missing integration tests though. I may look into adding them later... for now I conducted a manual testing of all feature (--force, --regex, --wildcard) against a number of repository types/format combinations.

This PR finally brings:

• 3 different asset matching policies: exact name, regex or wildcard
• confirmation before delete

In this MR I'm erasing the current "del|delete" command implementation, which does not go through the "api" module and is missing all above features... let me know if that's ok for you.

[f18m]

In this MR I'm erasing the current "del|delete" command implementation, which does not go through the "api" module and is missing all above features... let me know if that's ok for you.

ping :)

[f18m]

In this MR I'm erasing the current "del|delete" command implementation

This should be ok, as long as it all still fits together in a sensible manner. I don't think it's the case here but, if there's an API breaking change, please bump the major version of the project.

I'll wait until there are passing tests before reviewing this again.

Ok thanks. I have some issues running the integration tests though - I will open a different ticket to avoid polluting this one.

[f18m]

In this MR I'm erasing the current "del|delete" command implementation

This should be ok, as long as it all still fits together in a sensible manner. I don't think it's the case here but, if there's an API breaking change, please bump the major version of the project.

ok I don't think this will be the case for this MR though

I'll wait until there are passing tests before reviewing this again.

ok sure I'm looking at the tests these days but I have a couple of doubts about the tests... it would be great to have some little guidance:

1. is the "tests/cli/test_delete" (which is failing right now) an integration test or not? It seems to run both when I issue pytest -m 'integration' and also when I run pytest -m 'not integration'

2. in that test is the Nexus response mocked ? Should that test run against the docker container of the Nexus or instead against a mocked Nexus?

3. since with this MR the delete command is changed to go through the API path and use a Groovy script to do the delete, do you think the test should be changed? and if so, can you point me to an example to lay out the new test?

Thanks and sorry for all the questions but I have never used pytest-mock and for me it's more difficult to follow the test code rather than the "production" code :)

Thanks

[bt-thiago]

1. is the "tests/cli/test_delete" (which is failing right now) an integration test or not?

Integration tests have a @pytest.mark.integration decorator.

in that test is the Nexus response mocked ? Should that test run against the docker container of the Nexus or instead against a mocked Nexus?

It's a unit test (not integration) so the response is mocked. There isn't a integration test for this because I don't need to test Nexus itself - I call their API and trust that they do the right thing.

However, since you're introducing a Groovy script to perform the deletion, we can no longer trust that the server side is doing the "right thing", so it would be a good idea to introduce integration tests.

... do you think the test should be changed? and if so, can you point me to an example to lay out the new test?

As above, yes. We should keep a unit test to validate the internal API interface and add integration tests to validate the "nexus API" (ie the groovy script) interface and implementation.

Thanks and sorry for all the questions but I have never used pytest-mock and for me it's more difficult to follow the test code rather than the "production" code :)

No worries; mocking can get complicated. You have spent considerable time on this - I appreciate it and I hope it's being useful as a learning experience. Ultimately I'm responsible for maintaining this code so please understand if I don't merge this straight-away once the tests are passing as I will probably want to review the code mode in depth and do some refactoring along the way.

[f18m]

It's a unit test (not integration) so the response is mocked. There isn't a integration test for this because I don't need to test Nexus itself - I call their API and trust that they do the right thing.

actually I think I found out that there is an integration test with exactly the same name (that's what confused me at the beginning) inside the tests/cli/test_cli.py file. I think that it was an integration test removing the whole contents of a folder tree - this is a feature that the groovy script of this MR currently does not support, so I modified the test to delete file by file (and added verification that at the end the repo is empty).

However, since you're introducing a Groovy script to perform the deletion, we can no longer trust that the server side is doing the "right thing", so it would be a good idea to introduce integration tests.

Right - I spent more time on the integration test rather than the unit test because I think in this case it's mostly useful the integration test.

No worries; mocking can get complicated. You have spent considerable time on this - I appreciate it and I hope it's being useful as a learning experience. Ultimately I'm responsible for maintaining this code so please understand if I don't merge this straight-away once the tests are passing as I will probably want to review the code mode in depth and do some refactoring along the way.

Sure right - just let me know if I can do something else to get this integrated. The thing is: in my company I'm introducing Nexus as new technology to store artifacts and we really need a tool like nexus3-cli to operate on the artefacts we store there. As we are in a sort of hurry I may try to build a Pypi package from my fork of nexus3-cli till this MR is fully integrated (also because we will need to add support for Docker repos to nexus3 shortly). This way I will be able to decouple this MR from the timelines of my needs...

Thanks!

[codecov[bot]]

Codecov Report

Merging #72 into master will decrease coverage by 1.72%. The diff coverage is 56.16%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage   88.03%   86.31%   -1.73%     
==========================================
  Files          24       24              
  Lines        1070     1118      +48     
==========================================
+ Hits          942      965      +23     
- Misses        128      153      +25

Impacted Files	Coverage Δ
src/nexuscli/nexus_client.py	88.64% <ø> (+0.84%)	:arrow_up:
src/nexuscli/cli/__init__.py	84% <ø> (ø)	:arrow_up:
src/nexuscli/cli/util.py	64.7% <0%> (-8.63%)	:arrow_down:
src/nexuscli/cli/root_commands.py	57% <43.9%> (-10.12%)	:arrow_down:
src/nexuscli/api/repository/collection.py	87.09% <85.18%> (-0.54%)	:arrow_down:
src/nexuscli/api/script/model.py	93.47% <0%> (+2.17%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7113d94...0506853. Read the comment docs.