Added new content subgroup to repository commands #194

gerrod3 · 2021-04-01T23:18:55Z

fixes: #171

@mdellweg I marked the current repository content commands as deprecated to give time for users to update their usage to the new commands, but I can remove them if you want me to.

I set up the factory to allow for plugins that have multiple content types (ansible) and for plugins that have content that requires multiple fields for uniqueness (ansible, file). This requires some complex dynamic option generation in the factory, but I think the tradeoff is worth for the ease of adding this content subgroup to new plugins.

mdellweg · 2021-04-02T08:01:42Z

pulpcore/cli/file/repository.py

@@ -112,7 +114,7 @@ def sync(
    )


-@repository.command()
+@repository.command(deprecated=True)


pulpcore/cli/common/generic.py

mdellweg

Left some comments.
I still need to convince myself, that the magic around the dynamic natural key content lookup is a good thing (or at least the best we can get...).

pulpcore/cli/common/context.py

pulpcore/cli/common/generic.py

mdellweg · 2021-04-11T11:58:30Z

pulpcore/cli/file/repository.py

@@ -82,6 +83,7 @@ def repository(ctx: click.Context, pulp_ctx: PulpContext, repo_type: str) -> Non
 repository.add_command(task_command(decorators=nested_lookup_options))
 repository.add_command(version_command(decorators=nested_lookup_options))
 repository.add_command(label_command(decorators=nested_lookup_options))
+repository.add_command(repository_content_command(contexts={"file": PulpFileContentContext}))


Maybe this should be "file:file".
In the end nothing prevents us from adding content managed by another plugin to a repository.
e.g., I always wanted to test, whether we can replace pulp_deb's GenericContent by FileContent (they are basically identical).

Is there nothing in the code to prevent this? I know the plugin's repository models has a CONTENT_TYPES list field, but I'm not sure if that's check when adding content. Going with that naming scheme could be a lot of extra typing for plugin's with more than one content type and I'm not sure if there's benefit for allowing cross-plugin content sharing.

We can probably add that later...if needed
Of course we would add the "file:" by default for the user.

tests/scripts/pulp_file/test_content.sh

gerrod3 · 2021-04-13T21:52:06Z

Left some comments.
I still need to convince myself, that the magic around the dynamic natural key content lookup is a good thing (or at least the best we can get...).

Have you seen this https://github.com/pulp/pulp-cli/pull/195/files#diff-2330f1d2bec4c88428af0e90f4ad6feff6a1663af7be7b9fedb645ed98846f68R218, I think I could do a similar thing with a group option variant. With a group option the options would be defined before hand and they could use a generic callback that has access to all options for the lookup to get the content context. I will take a look into this and see if I can simplify the crazy magic code.

mdellweg · 2021-04-14T14:38:54Z

Left some comments.
I still need to convince myself, that the magic around the dynamic natural key content lookup is a good thing (or at least the best we can get...).

Have you seen this https://github.com/pulp/pulp-cli/pull/195/files#diff-2330f1d2bec4c88428af0e90f4ad6feff6a1663af7be7b9fedb645ed98846f68R218, I think I could do a similar thing with a group option variant. With a group option the options would be defined before hand and they could use a generic callback that has access to all options for the lookup to get the content context. I will take a look into this and see if I can simplify the crazy magic code.

Hmm, Something like that may actually work. Better yet, this mutex option will show up in the Help screen nonetheless. And only bug out in the parsing step. Worth a try.

mdellweg

Again, a lot to digest here...
But looking at the amount of code to add to the file section, i must say that it is pretty much very descriptive and not much boiler plate. 👍

mdellweg · 2021-04-21T08:29:03Z

pulpcore/cli/common/generic.py

+        repo_ctx.modify(repo_ctx.pulp_href, add_content=[content_ctx.pulp_href], base_version=bv)
+
+    @click.command("remove")
+    @click.option("--all", is_flag=True, help=_("Remove all content from repository version"))


Oh wow, that is new. ;)

pulpcore/cli/common/generic.py

pulpcore/cli/common/context.py

pulpcore/cli/file/context.py

pulpcore/cli/common/generic.py

pulpcore/cli/python/context.py

mdellweg

Sorry, the size of the PR is probably the reason i find something new every time i look at it.

CHANGES/171.feature

mdellweg · 2021-04-28T07:05:25Z

pulpcore/cli/common/generic.py

+        value = opts.get(self.name)
+        if self.callback is not None:
+            value = self.callback(ctx, self, {o: opts[o] for o in all_options})  # type: ignore
+        if self.expose_value:
+            ctx.params[self.name] = value
+        return value, args


I didn't look into the upstream implementation, but is this something, we could do with a super().handle_parse_result()?

No. super().handle_parse_result() does some casting and ensures the value passed in is a string that can be converted to the option's type. There is no easy way to ensure that the conversion won't mess up the combined value.

Ah i see, this is combining the value. Is this parsing step not called for each command in the group? how is this not clashing?

This parsing step is called for each option in the group, but only the option that has the callback will get the final value from processing each value in the group. This does mean that you can't supply defaults to these grouped options as the defaults are only filled in from super().handle_parse_result(). However, you can have a callback for each option in the group and all those callbacks will receive the values for each option in the group.

mdellweg · 2021-04-28T07:10:50Z

pulpcore/cli/common/generic.py

+        bv = f"{repo_ctx.pulp_href}versions/{base_version}/" if base_version is not None else None
+        repo_ctx.modify(repo_ctx.pulp_href, add_content=[content_ctx.pulp_href], base_version=bv)


Suggested change

bv = f"{repo_ctx.pulp_href}versions/{base_version}/" if base_version is not None else None

repo_ctx.modify(repo_ctx.pulp_href, add_content=[content_ctx.pulp_href], base_version=bv)

base_version_href = f"{repo_ctx.pulp_href}versions/{base_version}/" if base_version is not None else None

repo_ctx.modify(repo_ctx.pulp_href, add_content=[content_ctx.pulp_href], base_version=base_version_href)

If we moved the first line to callback that returns a RepositoryVersionContext, we would get another GET call, but also a better error message if the base_version did not exist. WDYT?

fixes: pulp#171 closes: pulp#215

mdellweg reviewed Apr 2, 2021

View reviewed changes

pulpcore/cli/file/repository.py

@@ -112,7 +114,7 @@ def sync(

)

@repository.command()

@repository.command(deprecated=True)

Copy link

Member

mdellweg Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

mdellweg reviewed Apr 6, 2021

View reviewed changes

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

gerrod3 force-pushed the content_factory branch from 1bcd01a to 3d7088d Compare April 6, 2021 14:28

mdellweg reviewed Apr 11, 2021

View reviewed changes

vonsch mentioned this pull request Apr 14, 2021

Add some handy pulp rpm commands #205

Closed

gerrod3 force-pushed the content_factory branch 5 times, most recently from f2250b0 to ef69910 Compare April 20, 2021 18:59

gerrod3 requested a review from mdellweg April 20, 2021 19:13

mdellweg reviewed Apr 21, 2021

View reviewed changes

daviddavis reviewed Apr 21, 2021

View reviewed changes

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

gerrod3 force-pushed the content_factory branch 3 times, most recently from 7b6eae9 to ed43993 Compare April 21, 2021 15:31

mdellweg added this to the 0.8.0 milestone Apr 22, 2021

mdellweg reviewed Apr 24, 2021

View reviewed changes

pulpcore/cli/python/context.py Outdated Show resolved Hide resolved

mdellweg approved these changes Apr 24, 2021

View reviewed changes

gerrod3 force-pushed the content_factory branch from ed43993 to 6f87a06 Compare April 27, 2021 14:17

mdellweg reviewed Apr 28, 2021

View reviewed changes

gerrod3 mentioned this pull request Apr 28, 2021

Deprecate old repository content add/remove/modify commands #215

Closed

gerrod3 force-pushed the content_factory branch 3 times, most recently from 1b1133e to 699d775 Compare April 28, 2021 15:09

Added new content subgroup to repository commands

89ee971

fixes: pulp#171 closes: pulp#215

gerrod3 force-pushed the content_factory branch from 699d775 to 89ee971 Compare April 28, 2021 20:08

mdellweg merged commit a3f3abe into pulp:develop Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new content subgroup to repository commands #194

Added new content subgroup to repository commands #194

gerrod3 commented Apr 1, 2021

mdellweg Apr 2, 2021

mdellweg left a comment

mdellweg Apr 11, 2021

gerrod3 Apr 13, 2021

mdellweg Apr 14, 2021

gerrod3 commented Apr 13, 2021

mdellweg commented Apr 14, 2021

mdellweg left a comment

mdellweg Apr 21, 2021

mdellweg left a comment

mdellweg Apr 28, 2021

gerrod3 Apr 28, 2021

mdellweg Apr 28, 2021

gerrod3 Apr 28, 2021

mdellweg Apr 28, 2021

mdellweg Apr 28, 2021

		bv = f"{repo_ctx.pulp_href}versions/{base_version}/" if base_version is not None else None
		repo_ctx.modify(repo_ctx.pulp_href, add_content=[content_ctx.pulp_href], base_version=bv)

Added new content subgroup to repository commands #194

Added new content subgroup to repository commands #194

Conversation

gerrod3 commented Apr 1, 2021

Choose a reason for hiding this comment

mdellweg 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

gerrod3 commented Apr 13, 2021

mdellweg commented Apr 14, 2021

mdellweg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdellweg 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