Add Support for ULN Remote in Pulp RPM #486

Manisha15 · 2022-03-11T08:23:33Z

No description provided.

pulpcore/cli/rpm/uln_remote.py

mdellweg

Can you please add tests for this?

pulpcore/cli/rpm/remote.py

mdellweg · 2022-03-14T07:55:48Z

Also, we need a changelog snippet (can point to this PR instead of an issue).

pulpcore/cli/rpm/remote.py

tests/scripts/pulp_rpm/test_uln_remote.sh

mdellweg · 2022-03-16T08:35:33Z

pulpcore/cli/rpm/context.py

+    ENTITIES = _("uln remotes")
+    HREF = "rpm_uln_remote_href"
+    ID_PREFIX = "remotes_rpm_uln"
+    NULLABLES = {


Any reason not to extend the set of the base class here?

Extended the RPm Remote base class and fixed Nullables as well.

OK, sorry i didn't express this properly. We can still safely inherit from PulpRemoteContext, while we would define NULLABLES = PulpRemoteContext.NULLABLES + {"uln_server-base_url"}.

But the question is, can we have nullable "uln_server-base_url"? In settings we set to default value and although its not a required field but we cannot set it to Null. Or am I understanding Nullables wrong?

OK that i don't know. NULLABLES is basically to translate the empty string provided if you specify --opt "" to None for submitting to the api.
Whether uln_server_base_url can be "unset" is a question for someone else. If it is not, i'd say maybe you don't need to redefine NULLABLES at all...
Also think about the update command. Maybe you need to provide a version of that option without default, or it will unintentionally be overwritten.

pulpcore/cli/rpm/context.py

mdellweg · 2022-03-18T09:48:40Z

You still need to spec fixes #470 in the commit message.

mdellweg · 2022-03-18T09:49:27Z

@ggainey can i kindly ask you to review this work for rpm-ness?

ggainey

Looks great! If we can get the requested changes to the test, I think we're good to go.

tests/scripts/pulp_rpm/test_uln_remote.sh

quba42

I found one issue while trying to locally test these changes, which is that the --remote option will not accept a ULN remote for either syncing an RPM repo, or for associating a RPM repo with a ULN remote. My test workflow goes as follows:

ENTITIES_NAME='test'
REMOTE_OPTIONS='--url=uln://ovm2_2.1.1_i386_patch --username=redacted --password=redacted'
pulp rpm remote --type=uln create --name=${ENTITIES_NAME} ${REMOTE_OPTIONS}
pulp rpm repository create --name=${ENTITIES_NAME} --remote=${ENTITIES_NAME}
or
pulp rpm repository create --name=${ENTITIES_NAME}
pulp rpm repository sync --name=${ENTITIES_NAME} --remote=${ENTITIES_NAME}

Both these workflows result in the following error:

Error: Could not find rpm remote with {'name': 'test'}.

Another less serious issue, is if we should provide some help text, for the various --url options used by the subcommands of ULN remote? Right now it is:

$ pulp rpm remote --type=uln create --help
Usage: pulp rpm remote create [OPTIONS]

  Create a uln remote.

Options:
  --name TEXT                     [required]
  --url TEXT                      [required]

But since for ULN remotes --url requires the ULN channel name e.g.: uln://ovm2_2.1.1_i386_patch (which is not very intuitive) some help text like the following would be great:

  --url TEXT                      [required] Use the ULN channel name starting with uln:// here.

Alternatively (perhaps even better), the CLI might check that any user supplied --url parameters start with uln:// for ULN remotes.

mdellweg · 2022-03-23T16:14:58Z

I found one issue while trying to locally test these changes, which is that the --remote option will not accept a ULN remote for either syncing an RPM repo, or for associating a RPM repo with a ULN remote. My test workflow goes as follows:
ENTITIES_NAME='test'
REMOTE_OPTIONS='--url=uln://ovm2_2.1.1_i386_patch --username=redacted --password=redacted'
pulp rpm remote --type=uln create --name=${ENTITIES_NAME} ${REMOTE_OPTIONS}
pulp rpm repository create --name=${ENTITIES_NAME} --remote=${ENTITIES_NAME}
or
pulp rpm repository create --name=${ENTITIES_NAME}
pulp rpm repository sync --name=${ENTITIES_NAME} --remote=${ENTITIES_NAME}
Both these workflows result in the following error:
Error: Could not find rpm remote with {'name': 'test'}.

I think, what you need to use is --remote uln:test or --remote rpm:uln:test.
I agree this is not well documented, but the default plugin:type combination for these repositories is rpm:rpm:....

Another less serious issue, is if we should provide some help text, for the various --url options used by the subcommands of ULN remote? Right now it is:
$ pulp rpm remote --type=uln create --help
Usage: pulp rpm remote create [OPTIONS]

  Create a uln remote.

Options:
  --name TEXT                     [required]
  --url TEXT                      [required]
But since for ULN remotes --url requires the ULN channel name e.g.: uln://ovm2_2.1.1_i386_patch (which is not very intuitive) some help text like the following would be great:
  --url TEXT                      [required] Use the ULN channel name starting with uln:// here.
Alternatively (perhaps even better), the CLI might check that any user supplied --url parameters start with uln:// for ULN remotes.

I'd say, let's go for both document and validate.

Manisha15 · 2022-03-24T08:09:13Z

pulpcore/cli/rpm/remote.py

+        allowed_with_contexts=(PulpUlnRemoteContext,),
+    ),
+    pulp_option(
+        "--url",


I have added the check for uln url as well as help_text. The only problem here now is we have two url options/fields in --help of uln remote. Is there a way to remove the base class url option?

Options: --name TEXT [required] --url TEXT [required] --ca-cert TEXT a PEM encoded CA certificate or @file containing same . . . --policy [immediate|on_demand|streamed] --uln-server-base-url TEXT ULN Server base URL, default is 'https://linux-update.oracle.com/' --url TEXT Use the ULN channel name starting with uln:// here. [required] --help Show this message and exit.

Well, the other --url option comes via the common_remote_{create,update}_options which are plain lists.
So maybe we can create a filtered copy of these [option for option in ... if option.name != "url"], and then add one --url option for the RpmRemoteContext and one for the UlnRemoteContext.
@gerrod3 may know whether they turn up correctly in the corresponding help outputs.

The common_remote_{create,update}_options are lists of decorators that come from click so they don't have argument name or at least callable arguments.

Then maybe it it impossible to use that shortcut here.

So where do you suggest to add this condition? Adding it to the base class doesn't look like a good idea.

Yeah, it's not so much a condition. I think we need to specify the used parameters explicitly here. Not particularly happy about this, but probably the only way to get this forward now.

You mean to remove generic common_remote_{create/update}_options options and define them explicitly in remote.py?

Yes, this is the way forward i can see today. And i know it's bad for consistency.

mdellweg · 2022-03-24T09:42:01Z

pulpcore/cli/rpm/remote.py

+        allowed_with_contexts=(PulpUlnRemoteContext,),
+    ),
+    pulp_option(
+        "--url",


Well, the other --url option comes via the common_remote_{create,update}_options which are plain lists.
So maybe we can create a filtered copy of these [option for option in ... if option.name != "url"], and then add one --url option for the RpmRemoteContext and one for the UlnRemoteContext.
@gerrod3 may know whether they turn up correctly in the corresponding help outputs.

mdellweg · 2022-03-24T09:43:18Z

pulpcore/cli/rpm/remote.py


 translation = get_translation(__name__)
 _ = translation.gettext

+def _url_callback(ctx: click.Context, param: click.Parameter, value: str) -> str:


Should this be named _uln_channel_callback or _uln_url_callback?

I would suggest _uln_url_callback as the callback is for url option.

Changed name from _url_callback to _uln_url_callback.

mdellweg · 2022-04-07T08:01:17Z

pulpcore/cli/rpm/remote.py

+    click.option("--sock-read-timeout", type=float),
+    click.option("--tls-validation", type=bool),
+    click.option("--total-timeout", type=float),
+    click.option("--username"),


Now you can merge the rpm_remote_options into these two lists with the proper values of required.

The options list will be repetitive. I can instead create list of unique remote_create_options, remote_update_options, and another list which have common options for create/update. What do you think?

Sounds good.

Modified the options list.

mdellweg · 2022-04-10T15:01:34Z

pulpcore/cli/rpm/remote.py

+    pulp_option(
+        "--uln-server-base-url",
+        default="https://linux-update.oracle.com/",
+        help=_("ULN Server base URL, default is 'https://linux-update.oracle.com/'"),
+        allowed_with_contexts=(PulpUlnRemoteContext,),
+    ),


I think, this needs to be split for create and update, because providing a default with update will always set this value, and you don't want that in the update call on an existing customized remote.

Added the changes.

fixes pulp#470

Manisha15 · 2022-04-11T10:04:53Z

Fixed the tests.

quba42

I was now able to fully verify the functionality (using this branch rebased to the latest pulp-cli main branch) using the following workflow:

Requires adding "sha1" to the allowed checksums in /etc/pulp/settings.py:
ALLOWED_CONTENT_CHECKSUMS = ["sha224", "sha256", "sha384", "sha512", "sha1"]
pstop
pulpcore-manager handle-artifact-checksums
pstart


ENTITIES_NAME='test'
REMOTE_OPTIONS='--url=uln://ovm2_2.1.1_i386_patch --username=redacted --password=redacted'
pulp rpm remote --type=uln create --name=${ENTITIES_NAME} ${REMOTE_OPTIONS}
pulp rpm repository create --name=${ENTITIES_NAME} --remote=uln:${ENTITIES_NAME}
pulp rpm repository sync --name=${ENTITIES_NAME}
RPM_PUBLICATION_HREF=$(pulp rpm publication create --repository=${ENTITIES_NAME} | jq -r '.pulp_href')
pulp rpm distribution create --name=${ENTITIES_NAME} --base-path=${ENTITIES_NAME} --publication=${RPM_PUBLICATION_HREF}

I also checked that the CLI now throws an error if the --url parameter does not start with uln://.

The --help option now also contains more explanations, thanks!

From a usage point of view this looks good to go to me!

ggainey

This looks great - thanks for all the work Manisha!

mdellweg reviewed Mar 11, 2022

View reviewed changes

pulpcore/cli/rpm/uln_remote.py Outdated Show resolved Hide resolved

Manisha15 force-pushed the add_uln_remote_suuport branch from ab9f01f to a8c354d Compare March 11, 2022 09:42

mdellweg reviewed Mar 14, 2022

View reviewed changes

pulpcore/cli/rpm/remote.py Outdated Show resolved Hide resolved

Manisha15 force-pushed the add_uln_remote_suuport branch 2 times, most recently from cc3cf09 to 44851c5 Compare March 16, 2022 06:59

mdellweg reviewed Mar 16, 2022

View reviewed changes

mdellweg linked an issue Mar 16, 2022 that may be closed by this pull request

Add support for ULN remotes #470

Closed

Manisha15 force-pushed the add_uln_remote_suuport branch 4 times, most recently from 0f2df16 to 4b9c60c Compare March 17, 2022 09:24

mdellweg reviewed Mar 17, 2022

View reviewed changes

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

Manisha15 force-pushed the add_uln_remote_suuport branch 3 times, most recently from 0026107 to 943cdf7 Compare March 17, 2022 13:36

mdellweg requested a review from ggainey March 17, 2022 13:59

Manisha15 force-pushed the add_uln_remote_suuport branch from 943cdf7 to abf0139 Compare March 17, 2022 14:20

ggainey requested changes Mar 18, 2022

View reviewed changes

tests/scripts/pulp_rpm/test_uln_remote.sh Show resolved Hide resolved

Manisha15 force-pushed the add_uln_remote_suuport branch 2 times, most recently from 1a2e128 to c245bf7 Compare March 21, 2022 09:04

mdellweg requested a review from ggainey March 23, 2022 10:37

quba42 suggested changes Mar 23, 2022

View reviewed changes

Manisha15 force-pushed the add_uln_remote_suuport branch from c245bf7 to 21dbead Compare March 24, 2022 08:05

Manisha15 commented Mar 24, 2022

View reviewed changes

mdellweg reviewed Mar 24, 2022

View reviewed changes

Manisha15 force-pushed the add_uln_remote_suuport branch from 21dbead to 5205520 Compare March 31, 2022 05:25

Manisha15 force-pushed the add_uln_remote_suuport branch from 5205520 to 4fb0fe6 Compare April 5, 2022 05:48

mdellweg reviewed Apr 7, 2022

View reviewed changes

Manisha15 force-pushed the add_uln_remote_suuport branch from 4fb0fe6 to 27038b7 Compare April 7, 2022 12:56

mdellweg reviewed Apr 10, 2022

View reviewed changes

Manisha15 force-pushed the add_uln_remote_suuport branch from 27038b7 to a535caf Compare April 11, 2022 05:22

Add Support for ULN Remote

b7a4b09

fixes pulp#470

Manisha15 force-pushed the add_uln_remote_suuport branch from a535caf to b7a4b09 Compare April 11, 2022 10:04

quba42 approved these changes Apr 14, 2022

View reviewed changes

ggainey approved these changes Apr 14, 2022

View reviewed changes

ggainey merged commit 067e4f6 into pulp:main Apr 14, 2022

mdellweg added this to the 0.15.0 milestone Apr 15, 2022

quba42 deleted the add_uln_remote_suuport branch July 26, 2022 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for ULN Remote in Pulp RPM #486

Add Support for ULN Remote in Pulp RPM #486

Manisha15 commented Mar 11, 2022

mdellweg left a comment

mdellweg commented Mar 14, 2022

mdellweg Mar 16, 2022

Manisha15 Mar 17, 2022

mdellweg Mar 17, 2022

Manisha15 Mar 17, 2022

mdellweg Mar 17, 2022

mdellweg commented Mar 18, 2022

mdellweg commented Mar 18, 2022

ggainey left a comment

quba42 left a comment •

edited

Loading

mdellweg commented Mar 23, 2022

Manisha15 Mar 24, 2022

mdellweg Mar 24, 2022

Manisha15 Mar 24, 2022

mdellweg Mar 30, 2022

Manisha15 Mar 30, 2022

mdellweg Mar 31, 2022

Manisha15 Mar 31, 2022

mdellweg Apr 4, 2022

mdellweg Mar 24, 2022

mdellweg Mar 24, 2022

Manisha15 Mar 30, 2022

Manisha15 Mar 31, 2022

mdellweg Apr 7, 2022

Manisha15 Apr 7, 2022

mdellweg Apr 7, 2022

Manisha15 Apr 7, 2022

mdellweg Apr 10, 2022

Manisha15 Apr 11, 2022

Manisha15 commented Apr 11, 2022

quba42 left a comment

ggainey left a comment

Add Support for ULN Remote in Pulp RPM #486

Add Support for ULN Remote in Pulp RPM #486

Conversation

Manisha15 commented Mar 11, 2022

mdellweg left a comment

Choose a reason for hiding this comment

mdellweg commented Mar 14, 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

mdellweg commented Mar 18, 2022

mdellweg commented Mar 18, 2022

ggainey left a comment

Choose a reason for hiding this comment

quba42 left a comment • edited Loading

Choose a reason for hiding this comment

mdellweg commented Mar 23, 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

Manisha15 commented Apr 11, 2022

quba42 left a comment

Choose a reason for hiding this comment

ggainey left a comment

Choose a reason for hiding this comment

quba42 left a comment •

edited

Loading