Delete yum artifacts regex #72

f18m · 2019-11-08T01:01:33Z

To be written

thiagofigueiro

Pending:

Unit and/or integration tests
linter fixes

thiagofigueiro · 2019-11-08T20:33:10Z

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]


Wouldn't these del_assets* subcommands be better under the existing root command (delete|del)?

nexus3 (delete|del) <repository_path>

You could add flags for the user to specify whether the <repository_path> contains a regex or a wildcard (glob?)

I'll have a better look at the problem this weekend so I have a better informed suggestion.

Regarding the 2 points:

subcommand vs root command: honestly as user of nexus3-cli I found very confusing having both subcommands AND root commands: I didn't pay much attention initially at the repository/script/etc subcommands documented at the end of the "--help" section and it took me a while to discover how many possibilities nexus3-cli provides beside the root commands... all in all they look at bit "hidden".
So IMHO I would completely remove root commands and leave only subcommands: all the list/upload/download/delete root commands IMHO should be under an "assets" subcommand.
Then in this scenario these del_assets_* commands added by this PR could be merged with the "nexus3 assets delete" command perhaps.

regarding having a single command instead of del_assets_regex/del_assets_wildcard: I think you're right, having a flag would make it possible to unify them.
Should the default be to interpret the parameter appearing after the <repo_name> as a wildcard or as a regex? Since wildcards are easier to use I would opt for the former.

All the commands used to be at the root level but I found that confusing as well. The help page was very long and most users want to upload/download/delete. So I decided to pack them in sub commands as this is already an established pattern in many cli utilities (git, for example).

As changing the CLI would be a breaking change, I’d favour a bit more research/discussion before acting on it. Perhaps build the docopt in a separate branch so we can see how it looks.

I agree wildcard makes more sense. There’s an existing behaviour where it deletes everything under a matching name if the name ends with a / (eg del directory/) - if possible, I’d like to keep this behaviour.

Thanks again for your contribution.

Ok sure - I understand that grouping root commands all together would be a breaking change... better not to mix with this MR. So, for now, should I keep the del_assets command under the "repository" subcommand? Or do you prefer to have a new subcommand or a root-level command for that?

I have to say that after some more attempts to use the new "del_assets_wildcard" command I discovered that the OrientDB used by Nexus3 is handling wildcards in a weird (at least to me) way: i.e. the wildcard symbol % matches only string prefixes or postfixes. E.g. if I have an asset named "folder1/myasset-1.2.3.rpm" and I provide as wildcard "folder1/%1.2.3%" the Groovy script will report 0 matches. If I provide "%1.2.3%", the asset will be matched.
All this to say: maybe allowing the user to use wildcards instead of regex is not that useful finally...

Yes please. I'd prefer to keep it consistent with the subcommand pattern. I'll take your feedback onboard and think of a way to better expose the subcommands to first-time users.

I think that's still useful - if we can clearly communicate how the wildcard handling works (or link to a place that does it). One thing that I didn't consider is what happens when a file you want to delete has a wildcard character in it. I guess the user would need to escape it by default, unless we only enable these options through flags; e.g.:

nexus3 (delete|del) <repository_path> [--regex|--wildcard] Options: --regex Enable regular expression parsing on <repository_path> --wildcard Enable wildcard parsing on <repository_path>

Hi @thiagofigueiro ,
just to be sure I understand: from reading the example docs you put in point #2 I assume that the answer w.r.t. point #1 is "having these del_assets_wildcard and del_assets_regex merged with the root-level 'del' command" right?

Hi @thiagofigueiro ,
I was going to implement the change but I realized now that regarding point 1

if you want to have the "del_assets_wildcard/regex" commands merged with the "delete|del" command there's a small detail to fix: former cmds take the repository name and, separated, the repository path. The latter cmd is taking just the repository path. So how are user supposed to delete assets by regex?
Would that be something like:
nexus3 del --regex myrepoName/^[a-z]*myregex.*
?
Or can we modify the "delete|del" command to separate the reponame from the asset name:
nexus3 del --regex myrepoName ^[a-z]*myregex.*
?

--regex and --wildcard would be toggle flags to the del command.

There's existing code that can receive this

nexus3 del --regex myrepoName/^[a-z]*myregex.*

and split it using something like this.

It's a part of the code that is older and somewhat messy but it does the job. Assuming this is the docopt line:

nexus3 (delete|del) <repository_path>

Then you could do something like this:

if options.get('--regex') or options.get('--wildcard'): repository, regex_or_wildcard = self._pop_repository(options.get('<repository_path>')) # delete wildcard or regex code else: # existing delete code

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...

PS: I pushed a preliminary (untested) version where I tried to merge my delete command with previous one... if you can have a look and tell me what you think before I do the testing that would be appreciated, thanks!

src/nexuscli/cli/subcommand_repository.py

…eview comment

thiagofigueiro · 2019-11-23T13:00:22Z

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 ***@***.***> 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 · 2019-11-25T22:16:30Z

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 · 2019-11-30T10:53:15Z

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 · 2019-12-01T10:21:32Z

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)...

Add repotype check (delete only from HOSTED repositories) Add enumeration to distinguish EXACT NAME, WILDCARD, REGEX asset matching techniques

and limited to exact name match)

f18m · 2019-12-14T20:52:40Z

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 · 2019-12-23T18:51:10Z

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 :)

bt-thiago

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.

src/nexuscli/api/repository/collection.py

bt-thiago · 2019-12-24T08:35:45Z

src/nexuscli/api/repository/collection.py

+        # parse the JSON we got back
+        if 'result' not in groovy_returned_json:
+            raise exception.NexusClientAPIError(groovy_returned_json)
+        script_result = json.loads(groovy_returned_json['result']) # this is actually a JSON: convert to Python dict


I believe there's a .json method in the requests' response that would be preferable to calling json.loads directly.

I think that ScriptCollection.run() is already doing .json() on the HTTP response from the Nexus. However I think that the response.result field is interpreted as a string and does not handle the case whether that string is another JSON... hence the json.loads()...

src/nexuscli/api/repository/collection.py

src/nexuscli/cli/root_commands.py

bt-thiago · 2019-12-24T08:53:12Z

src/nexuscli/cli/root_commands.py

+            return errors.CliReturnCode.API_ERROR.value
+
+        if len(assets_list) == 0:
+            sys.stdout.write('Found 0 matching assets: aborting delete\n')


I think the existing message still applies here. The "aborting" wording is a bit confusing as it didn't abort but merely didn't find anything to delete.

Existing message:

file_word = PLURAL('file', delete_count) sys.stderr.write(f'Deleted {delete_count} {file_word}\n')

ok I changed that - however I'm not sure why "stderr" is used here... if that's not an error it should probably go in stdout like other messages...

src/nexuscli/cli/root_commands.py

bt-thiago · 2019-12-24T08:58:17Z

src/nexuscli/cli/root_commands.py

+def _cmd_del_assets(nexus_client, repoName, assetName, assetMatchOption, doForce):
+    """Performs ``nexus3 repository delete_assets``"""
+
+    nl = '\n' # see https://stackoverflow.com/questions/44780357/how-to-use-newline-n-in-f-string-to-format-output-in-python-3-6


I don't see this being used. It looks pretty hacky anyway, so probably for the best :)

right - was used in a previous attempt - deleted now

actually this is used: in 2 print() commands to show the list of deleted files I'm doing: nl.join(assets_list) to get a newline-separated list of files...

src/nexuscli/nexus_util.py

f18m · 2019-12-24T12:29:02Z

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 · 2019-12-29T15:50:21Z

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:

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'
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?
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 · 2019-12-30T08:22:26Z

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.

…he repo at the end of the test is indeed empty

f18m · 2019-12-30T16:36:01Z

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 · 2019-12-30T16:50:39Z

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%)`	⬆️
src/nexuscli/cli/__init__.py	`84% <ø> (ø)`	⬆️
src/nexuscli/cli/util.py	`64.7% <0%> (-8.63%)`	⬇️
src/nexuscli/cli/root_commands.py	`57% <43.9%> (-10.12%)`	⬇️
src/nexuscli/api/repository/collection.py	`87.09% <85.18%> (-0.54%)`	⬇️
src/nexuscli/api/script/model.py	`93.47% <0%> (+2.17%)`	⬆️

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.

fmontorsi added 2 commits November 8, 2019 01:42

Add repository del_assets command

6ac9c06

Add variant to use wildcard

0cca23d

thiagofigueiro requested changes Nov 8, 2019

View reviewed changes

fmontorsi added 2 commits November 9, 2019 00:43

Improve error handling from Groovy script; use f-strings to address r…

a67c777

…eview comment

Merge new delete command with previous one

6f19be9

fmontorsi and others added 3 commits November 23, 2019 18:47

Merge branch 'master' into delete_yum_artifacts_regex

26e45ce

fix whitespaces

6569a9c

Fix some obvious problems

d8913b6

fmontorsi and others added 3 commits December 14, 2019 12:21

Merge branch 'master' into delete_yum_artifacts_regex

cb9d3a5

Restore functionalities after integrating fixes from the MASTER branch.

4087402

Add repotype check (delete only from HOSTED repositories) Add enumeration to distinguish EXACT NAME, WILDCARD, REGEX asset matching techniques

Remove previous delete command implementation (not going through APIs

b987072

and limited to exact name match)

Fix linter warnings

d7e6ebf

f18m requested a review from thiagofigueiro December 14, 2019 20:54

Fix for tests

806cfa0

bt-thiago suggested changes Dec 24, 2019

View reviewed changes

f18m added 3 commits December 29, 2019 17:27

Address reviewers comments

acf6586

Fix integration tests

32e59be

Fix unit tests

0ccbed4

Fix flake8 errors/warnings; improve integration test to verify that t…

2a70340

…he repo at the end of the test is indeed empty

Merge branch 'master' into delete_yum_artifacts_regex

1c9fabe

f18m added 2 commits December 30, 2019 17:40

Fix more flake8 errors

0b1d9da

Fix CodeFactor error

b9fe5cc

more flake8 fixes

0506853

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete yum artifacts regex #72

Delete yum artifacts regex #72

f18m commented Nov 8, 2019

thiagofigueiro left a comment

thiagofigueiro Nov 8, 2019

f18m Nov 8, 2019 •

edited

Loading

thiagofigueiro Nov 9, 2019

f18m Nov 9, 2019

thiagofigueiro Nov 9, 2019

f18m Nov 19, 2019 •

edited

Loading

f18m Nov 21, 2019

thiagofigueiro Nov 22, 2019 •

edited

Loading

f18m Nov 23, 2019 •

edited

Loading

thiagofigueiro commented Nov 23, 2019 via email

f18m commented Nov 25, 2019

f18m commented Nov 30, 2019

f18m commented Dec 1, 2019

f18m commented Dec 14, 2019

f18m commented Dec 23, 2019

bt-thiago left a comment

bt-thiago Dec 24, 2019

f18m Dec 29, 2019

bt-thiago Dec 24, 2019

f18m Dec 29, 2019

bt-thiago Dec 24, 2019

f18m Dec 29, 2019

f18m Dec 29, 2019

f18m commented Dec 24, 2019

f18m commented Dec 29, 2019

bt-thiago commented Dec 30, 2019

f18m commented Dec 30, 2019

codecov bot commented Dec 30, 2019 •

edited

Loading

Delete yum artifacts regex #72

Are you sure you want to change the base?

Delete yum artifacts regex #72

Conversation

f18m commented Nov 8, 2019

thiagofigueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f18m Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f18m Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagofigueiro Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

f18m Nov 23, 2019 • edited Loading

Choose a reason for hiding this comment

thiagofigueiro commented Nov 23, 2019 via email

f18m commented Nov 25, 2019

f18m commented Nov 30, 2019

f18m commented Dec 1, 2019

f18m commented Dec 14, 2019

f18m commented Dec 23, 2019

bt-thiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f18m commented Dec 24, 2019

f18m commented Dec 29, 2019

bt-thiago commented Dec 30, 2019

f18m commented Dec 30, 2019

codecov bot commented Dec 30, 2019 • edited Loading

Codecov Report

f18m Nov 8, 2019 •

edited

Loading

f18m Nov 19, 2019 •

edited

Loading

thiagofigueiro Nov 22, 2019 •

edited

Loading

f18m Nov 23, 2019 •

edited

Loading

codecov bot commented Dec 30, 2019 •

edited

Loading