Make the --requirements param to accept file and text #608

lubosmj · 2022-11-30T18:13:18Z

closes #230

mdellweg · 2022-12-01T10:58:35Z

pulpcore/cli/common/generic.py

@@ -328,74 +328,59 @@ def _version_callback(
    return value


-# TODO: would be great to have enable this to take a validator, rather than having
-# to build "on top of" it like I'm doing now w/ json_callback
 def load_file_or_string_callback(


I'm not grocking the whole change yet, but this thing now looks deceivingly like a decorator. Do you think we can use it as one?

Sure thing, sir!

So this one can be used as a callback or chained when used as a decorator...
Maybe that should be in the docstring.

mdellweg · 2022-12-05T13:41:39Z

pulpcore/cli/ansible/remote.py

-        callback=_requirements_callback,
+        callback=lambda c, p, x: f"{yaml.safe_load(x)}" if x else x,


Guess this is not what we wanted...

mdellweg · 2022-12-05T13:43:17Z

pulpcore/cli/ansible/repository.py

-content_json_callback = create_content_json_callback(schema=CONTENT_LIST_SCHEMA)
+


Is there a reason not to reuse the generated callback anymore?

I do not understand. I am using literally the same callback as before a couple of lines below.

mdellweg · 2022-12-05T13:52:11Z

pulpcore/cli/common/generic.py

+@load_file_or_string_callback
+def string_callback(ctx: click.Context, param: click.Parameter, value: str) -> str:
+    return value


This is just like using the load_file_or_string_callback directly.

mdellweg · 2022-12-05T13:56:31Z

pulpcore/cli/common/generic.py

-    value = load_json_callback(ctx, param, value)
+@load_file_or_string_callback
+def labels_callback(ctx: click.Context, param: click.Parameter, value: str) -> Any:
+    value = json_callback(ctx, param, value)


Maybe json_callback should follow the chaining pattern. Also this version of json_callback is already decorated. So we are attemting to lookup a file twice here.

I think, i'd be ok to use the load_json_callback here. We never want a version of the label callback that cannot read from file, right?

Makes perfect sense. Thanks!

pulpcore/cli/common/generic.py

mdellweg · 2022-12-05T13:59:46Z

pulpcore/cli/common/generic.py

-    callback=load_json_callback,
+    callback=json_callback,


What do you think about the following naming convention?
load_string_callback searches for "@" and attempts to load from file else value.
json_callback transforms value to json.
load_json_callback is json_callback decorated (chained) with load_string_callback.

string_callback would be the same as specifying nothing...

pulpcore/cli/ansible/remote.py

mdellweg · 2022-12-15T13:55:04Z

pulpcore/cli/ansible/remote.py

-        callback=_requirements_callback,
+        callback=requirements_callback,


Has this ever been correct to to use the requirements callback? Also we should rename that one as all it does is load some yaml. Let's call it the yaml_callback.

mdellweg · 2022-12-15T13:55:30Z

pulpcore/cli/ansible/remote.py

        help=_("Collections only: URL to receive a session token"),
        allowed_with_contexts=collection_context,
    ),
    pulp_option(
        "--token",
-        callback=_requirements_callback,
+        callback=requirements_callback,


mdellweg · 2022-12-15T13:58:17Z

pulpcore/cli/common/generic.py

-# to build "on top of" it like I'm doing now w/ json_callback
-def load_file_or_string_callback(
-    ctx: click.Context, param: click.Parameter, value: Optional[str]
+def load_file_or_string_wrapper(


Let's call this the load_file_wrapper.

mdellweg · 2022-12-15T13:58:46Z

pulpcore/cli/common/generic.py

-    # None or empty-str are legal - shortcircuit here
-    if not value:
-        return value
+load_file_or_string_callback = load_file_or_string_wrapper()


this one the load_string_callback

mdellweg · 2022-12-15T13:59:12Z

pulpcore/cli/common/generic.py

-    # None is legal - shortcircuit here
-    if value is None:
-        return value
+load_json_callback = load_file_or_string_wrapper(json_callback)


and they'd be in line with this one.

mdellweg · 2022-12-15T14:01:22Z

pulpcore/cli/common/generic.py

-    value = load_json_callback(ctx, param, value)
+@load_file_or_string_callback
+def labels_callback(ctx: click.Context, param: click.Parameter, value: str) -> Any:
+    value = json_callback(ctx, param, value)


I think, i'd be ok to use the load_json_callback here. We never want a version of the label callback that cannot read from file, right?

mdellweg · 2022-12-15T14:03:47Z

pulpcore/cli/common/generic.py

-    callback=load_json_callback,
+    callback=load_file_or_string_callback,


The help text states we want JSON...

mdellweg · 2023-01-10T12:37:52Z

pulpcore/cli/ansible/remote.py

-        callback=_requirements_callback,
+        callback=yaml_callback,


I think this was a copy and paste error in the first place. The original callback didn't do a thing for this option (look at the if-elif structure of the code you removed).
Also we really expect just a string for a url, no?

mdellweg · 2023-01-10T12:38:02Z

pulpcore/cli/ansible/remote.py

-        callback=_requirements_callback,
+        callback=yaml_callback,


mdellweg · 2023-01-10T13:02:12Z

pulpcore/cli/common/generic.py

-def load_file_or_string_callback(
-    ctx: click.Context, param: click.Parameter, value: Optional[str]
+def load_file_wrapper(
+    handler: Callable[[click.Context, click.Parameter, str], Any] = lambda c, p, x: x


I think, i'd be better to not have the default value here, but pass the lambda in line 364.
load_string_callback = load_file_wrapper(lambda c, p, x: x)

mdellweg · 2023-01-10T13:05:03Z

pulpcore/cli/common/generic.py

-    callback=labels_callback,
+    callback=load_json_callback,


The existing labels_callback provides valuable validation. Please keep it.

I thought you wanted to remove it by saying this: #608 (comment). My bad. I did this now:

def load_labels_callback(ctx: click.Context, param: click.Parameter, value: str) -> Any: value = load_json_callback(ctx, param, value)

So labels are optionally read from file and always validated? Sounds right to me.

mdellweg · 2023-01-10T13:06:54Z

pulpcore/cli/file/repository.py

-        return None
+@load_file_wrapper
+def _content_list_callback(ctx: click.Context, param: click.Parameter, value: str) -> Any:
+    result = json_callback(ctx, param, value)


Don't we still need the

if result is None: return None

part?

Yes, we do! Thanks!

mdellweg · 2023-01-18T10:51:20Z

pulpcore/cli/common/generic.py

+load_json_callback = load_file_wrapper(json_callback)
+
+
+def load_labels_callback(ctx: click.Context, param: click.Parameter, value: str) -> Any:


We can be more specific here.

Suggested change

def load_labels_callback(ctx: click.Context, param: click.Parameter, value: str) -> Any:

def load_labels_callback(ctx: click.Context, param: click.Parameter, value: str) -> Optional[Dict[str, str]]:

(Don't forget to run black...)
Also when you change the name, can you verify that this is not used anywhere in the known plugins?

This should be my last request. We are ready to go then...

I ran git grep "labels_callback" against pulp-cli, pulp-cli-deb, and pulp-cli-ostree. Only these occurrences were found:

pulpcore/cli/common/generic.py:def load_labels_callback( pulpcore/cli/common/generic.py: callback=load_labels_callback,

I think we are good to go.

closes pulp#230

lubosmj · 2023-01-18T14:12:45Z

I have made a couple of value parameters to receive Optional[str] values. I hope it is alright.

lubosmj force-pushed the requirements-option-accept-str-file-230 branch 3 times, most recently from 471d9ba to 9b7a26c Compare December 1, 2022 10:30

mdellweg reviewed Dec 1, 2022

View reviewed changes

lubosmj force-pushed the requirements-option-accept-str-file-230 branch 6 times, most recently from 3b9c5f3 to febb165 Compare December 2, 2022 19:15

lubosmj marked this pull request as ready for review December 2, 2022 19:49

mdellweg reviewed Dec 5, 2022

View reviewed changes

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

mdellweg reviewed Dec 5, 2022

View reviewed changes

lubosmj marked this pull request as draft December 5, 2022 20:28

lubosmj force-pushed the requirements-option-accept-str-file-230 branch from febb165 to aa56f4c Compare December 12, 2022 17:54

lubosmj marked this pull request as ready for review December 13, 2022 19:59

lubosmj marked this pull request as draft December 15, 2022 13:00

lubosmj force-pushed the requirements-option-accept-str-file-230 branch 3 times, most recently from 546ffa8 to 663c4ae Compare December 15, 2022 13:19

lubosmj marked this pull request as ready for review December 15, 2022 13:39

mdellweg requested changes Dec 15, 2022

View reviewed changes

lubosmj marked this pull request as draft December 15, 2022 17:37

lubosmj force-pushed the requirements-option-accept-str-file-230 branch from 663c4ae to 8e5e8df Compare January 8, 2023 17:23

lubosmj marked this pull request as ready for review January 8, 2023 18:14

lubosmj requested a review from mdellweg January 8, 2023 18:14

mdellweg reviewed Jan 10, 2023

View reviewed changes

lubosmj marked this pull request as draft January 12, 2023 17:26

lubosmj force-pushed the requirements-option-accept-str-file-230 branch from 8e5e8df to 966867e Compare January 16, 2023 21:43

lubosmj marked this pull request as ready for review January 16, 2023 22:42

lubosmj requested a review from mdellweg January 16, 2023 22:42

mdellweg reviewed Jan 18, 2023

View reviewed changes

lubosmj force-pushed the requirements-option-accept-str-file-230 branch 2 times, most recently from 8150c53 to 4eae390 Compare January 18, 2023 14:10

Make the --requirements param to accept file and text

1b77253

closes pulp#230

lubosmj force-pushed the requirements-option-accept-str-file-230 branch from 4eae390 to 1b77253 Compare January 18, 2023 14:12

mdellweg approved these changes Jan 18, 2023

View reviewed changes

mdellweg enabled auto-merge (rebase) January 18, 2023 14:15

mdellweg merged commit 7fd4fa3 into pulp:main Jan 18, 2023

mdellweg added this to the 0.17.0 milestone Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the --requirements param to accept file and text #608

Make the --requirements param to accept file and text #608

lubosmj commented Nov 30, 2022

mdellweg Dec 1, 2022

lubosmj Dec 1, 2022

mdellweg Dec 5, 2022

mdellweg Dec 5, 2022

mdellweg Dec 5, 2022

lubosmj Dec 5, 2022

mdellweg Dec 5, 2022

mdellweg Dec 5, 2022

mdellweg Dec 15, 2022

lubosmj Dec 15, 2022

mdellweg Dec 5, 2022

mdellweg Dec 5, 2022

mdellweg Dec 15, 2022

mdellweg Dec 15, 2022

mdellweg Dec 15, 2022

mdellweg Dec 15, 2022

mdellweg Dec 15, 2022

mdellweg Dec 15, 2022

mdellweg Dec 15, 2022

mdellweg Jan 10, 2023

mdellweg Jan 10, 2023

mdellweg Jan 10, 2023

mdellweg Jan 10, 2023

lubosmj Jan 16, 2023

mdellweg Jan 18, 2023

mdellweg Jan 10, 2023

lubosmj Jan 16, 2023 •

edited

Loading

mdellweg Jan 18, 2023

lubosmj Jan 18, 2023 •

edited

Loading

lubosmj commented Jan 18, 2023

		callback=_requirements_callback,
		callback=lambda c, p, x: f"{yaml.safe_load(x)}" if x else x,

		content_json_callback = create_content_json_callback(schema=CONTENT_LIST_SCHEMA)

		callback=_requirements_callback,
		callback=requirements_callback,

		callback=load_json_callback,
		callback=load_file_or_string_callback,

		load_json_callback = load_file_wrapper(json_callback)


		def load_labels_callback(ctx: click.Context, param: click.Parameter, value: str) -> Any:

Make the --requirements param to accept file and text #608

Make the --requirements param to accept file and text #608

Conversation

lubosmj commented Nov 30, 2022

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubosmj Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubosmj Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

lubosmj commented Jan 18, 2023

lubosmj Jan 16, 2023 •

edited

Loading

lubosmj Jan 18, 2023 •

edited

Loading