Repository version user-entered options order fix #847

MichalPysik · 2023-12-07T12:07:31Z

The order in which the user enters options (--version, --repository, ..) no longer matters for
'pulp repository version' commands, since all API calls are now deferred to after the
options' callbacks are processed.

closes #650

Review Checklist:

An issue is properly linked. [feature and bugfix only]
Tests are present or not feasible.
Commits are split in a logical way (not historically).

MichalPysik · 2023-12-07T13:52:24Z

I don't know whether that's needed, but if you want me to add a simple test case, I can insert a pulp {plugin} repository version show somewhere in the existing tests.

lubosmj · 2023-12-07T15:36:08Z

You may sneak a repository version destroy call at the end of an existing test, like: pulp file repository version destroy --version 1 --repository=repo and change the order of some of the used resource lookups, like:

pulp-cli/tests/scripts/pulp_rpm/test_rpm_sync_publish.sh

Line 101 in 3183a2a

    
           expect_succ pulp rpm repository version repair --repository "cli_test_rpm_repository" --version 1

,

pulp-cli/tests/scripts/pulp_container/test_copy.sh

Line 32 in 3183a2a

    
           expect_succ pulp container repository copy-manifest --repository "cli_test_dest_container_repository" --source "cli_test_source_container_repository" --version "1" --media-type "application/vnd.docker.distribution.manifest.v2+json"

.

With this, we will verify whether your fix works globally or not.

daviddavis · 2023-12-11T13:24:02Z

CHANGES/650.bugfix

@@ -0,0 +1 @@
+Option (also resource lookup option) --repository is now an eager (high priority)  parameter.


A user reading through the changelog would have no idea what this means.

@daviddavis thanks, you're right. Is the new one any better?

mdellweg · 2023-12-11T15:26:49Z

pulpcore/cli/common/generic.py

@@ -863,11 +863,13 @@ def _type_callback(ctx: click.Context, param: click.Parameter, value: t.Optional
    help=_("Name of the repository"),
    callback=lookup_callback("name", PulpRepositoryContext),
    expose_value=False,
+    is_eager=True,


I'm not convinced, this is a nice solution, because it put's --repository on the same level as --help.
I believe we have ways to specify repository versions in a single option with the export code.

Would that mean that the help message will not be printed if I use both --repository and --help?

That's sure something to test. But I'm hesitant to call "--repository" that special.

@mdellweg I though so based on the documentation, but --help still acts as it should. Even if I use --repository with an invalid repository name, and put --help AFTER --repository, I still only get the help message printed.

Maybe --help is not only eager, but also has some special property in Click.

"repo" is a repository that actually exists, "repo2" is nonexistent

pulp file repository version show --repository repo --help Usage: pulp file repository version show [OPTIONS] Show details of a repository version. Options: --repository-href TEXT HREF of the repository --repository TEXT A resource to look for identified by <name> or by <href>. --version INTEGER Repository version number --help Show this message and exit.

pulp file repository version show --repository repo2 --help Usage: pulp file repository version show [OPTIONS] Show details of a repository version. Options: --repository-href TEXT HREF of the repository --repository TEXT A resource to look for identified by <name> or by <href>. --version INTEGER Repository version number --help Show this message and exit.

Also, adding --version ANYWHERE doesn't change the outcomes.

OK, eager is not the solution, because the real issue in disguise is that _version_callback is evaluating the repository context prematurely (before all options have been seen), by accessing it's pulp_href. Leading to api calls before we even know the command is syntactically correct.
Instead, it should hook the contexts up lazily.

mdellweg · 2024-01-12T16:49:41Z

pulpcore/cli/common/generic.py

@@ -863,11 +863,13 @@ def _type_callback(ctx: click.Context, param: click.Parameter, value: t.Optional
    help=_("Name of the repository"),
    callback=lookup_callback("name", PulpRepositoryContext),
    expose_value=False,
+    is_eager=True,


OK, eager is not the solution, because the real issue in disguise is that _version_callback is evaluating the repository context prematurely (before all options have been seen), by accessing it's pulp_href. Leading to api calls before we even know the command is syntactically correct.
Instead, it should hook the contexts up lazily.

pulpcore/cli/common/generic.py

tests/scripts/pulp_rpm/test_rpm_sync_publish.sh

pulp-glue/pulp_glue/common/context.py

tests/test_help_pages.py

The order in which the user enters options (--version, --repository, ..) no longer matters for 'pulp <plugin> repository version' commands, since all API calls are now deferred to after the options' callbacks are processed. closes pulp#650

MichalPysik requested review from mdellweg, lubosmj and gerrod3 December 7, 2023 12:24

MichalPysik force-pushed the repository_argument_order_fix branch 2 times, most recently from 202cfbc to 09f90d9 Compare December 11, 2023 12:28

MichalPysik changed the title ~~Resource lookup option --repository is now an eager parameter.~~ Option --repository is now an eager parameter. Dec 11, 2023

lubosmj approved these changes Dec 11, 2023

View reviewed changes

daviddavis reviewed Dec 11, 2023

View reviewed changes

mdellweg reviewed Dec 11, 2023

View reviewed changes

MichalPysik force-pushed the repository_argument_order_fix branch from 09f90d9 to 3a999ae Compare December 15, 2023 09:01

MichalPysik requested review from mdellweg and daviddavis December 15, 2023 09:15

mdellweg requested changes Jan 12, 2024

View reviewed changes

MichalPysik force-pushed the repository_argument_order_fix branch from 3a999ae to d0a9e41 Compare January 22, 2024 10:24

MichalPysik force-pushed the repository_argument_order_fix branch from d0a9e41 to 51c8564 Compare February 1, 2024 16:14

MichalPysik changed the title ~~Option --repository is now an eager parameter.~~ Repository version user-entered options order fix Feb 1, 2024

MichalPysik requested a review from mdellweg February 1, 2024 16:16

mdellweg reviewed Feb 1, 2024

View reviewed changes

pulpcore/cli/common/generic.py Show resolved Hide resolved

mdellweg mentioned this pull request Feb 2, 2024

Find does not check for uniquenes as advertised. #894

Closed

MichalPysik force-pushed the repository_argument_order_fix branch 3 times, most recently from 7f96cbc to dcbf95f Compare February 12, 2024 12:14

MichalPysik requested a review from mdellweg February 12, 2024 12:51

MichalPysik force-pushed the repository_argument_order_fix branch 3 times, most recently from 493b594 to 6cc4258 Compare February 15, 2024 09:44

mdellweg removed request for daviddavis and mdellweg February 15, 2024 11:28

mdellweg removed the request for review from gerrod3 February 15, 2024 11:28

MichalPysik force-pushed the repository_argument_order_fix branch from 6cc4258 to f67ef63 Compare February 16, 2024 11:51

mdellweg reviewed Feb 16, 2024

View reviewed changes

tests/scripts/pulp_rpm/test_rpm_sync_publish.sh Outdated Show resolved Hide resolved

MichalPysik force-pushed the repository_argument_order_fix branch 10 times, most recently from 15aa4b6 to c537cbb Compare February 19, 2024 14:34

mdellweg reviewed Feb 20, 2024

View reviewed changes

pulp-glue/pulp_glue/common/context.py Outdated Show resolved Hide resolved

tests/test_help_pages.py Outdated Show resolved Hide resolved

MichalPysik force-pushed the repository_argument_order_fix branch 4 times, most recently from 0e7552b to f6ded88 Compare February 22, 2024 12:04

MichalPysik force-pushed the repository_argument_order_fix branch from f6ded88 to 807416a Compare February 22, 2024 12:35

mdellweg approved these changes Feb 22, 2024

View reviewed changes

mdellweg merged commit 9a061aa into pulp:main Feb 22, 2024
20 checks passed

MichalPysik deleted the repository_argument_order_fix branch February 26, 2024 10:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repository version user-entered options order fix #847

Repository version user-entered options order fix #847

MichalPysik commented Dec 7, 2023 •

edited

Loading

MichalPysik commented Dec 7, 2023

lubosmj commented Dec 7, 2023

daviddavis Dec 11, 2023

MichalPysik Dec 15, 2023

mdellweg Dec 11, 2023

lubosmj Dec 11, 2023

mdellweg Dec 13, 2023

MichalPysik Dec 15, 2023 •

edited

Loading

MichalPysik Dec 15, 2023 •

edited

Loading

mdellweg Jan 12, 2024

mdellweg Jan 12, 2024

		@@ -0,0 +1 @@
		Option (also resource lookup option) --repository is now an eager (high priority) parameter.

Repository version user-entered options order fix #847

Repository version user-entered options order fix #847

Conversation

MichalPysik commented Dec 7, 2023 • edited Loading

MichalPysik commented Dec 7, 2023

lubosmj commented Dec 7, 2023

daviddavis Dec 11, 2023

Choose a reason for hiding this comment

MichalPysik Dec 15, 2023

Choose a reason for hiding this comment

mdellweg Dec 11, 2023

Choose a reason for hiding this comment

lubosmj Dec 11, 2023

Choose a reason for hiding this comment

mdellweg Dec 13, 2023

Choose a reason for hiding this comment

MichalPysik Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

MichalPysik Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

mdellweg Jan 12, 2024

Choose a reason for hiding this comment

mdellweg Jan 12, 2024

Choose a reason for hiding this comment

MichalPysik commented Dec 7, 2023 •

edited

Loading

MichalPysik Dec 15, 2023 •

edited

Loading

MichalPysik Dec 15, 2023 •

edited

Loading