Fix '--signing-service' option not allowing HREF#39
Fix '--signing-service' option not allowing HREF#39tjmullicani wants to merge 1 commit intopulp:mainfrom
Conversation
|
@hstct If you can find the time to test this locally, I am fine with merging it as is. |
|
Now that the tests in Normally I would also say we should add a test for the new option, but given we do not yet have any test involving signing service creation, I think this would be a bit much to ask. Instead, I have opened a new issue: #46 |
quba42
left a comment
quba42
left a comment
There was a problem hiding this comment.
When I test this locally (rebased on to latest main branch) I get the following error:
$ pulp deb publication create --simple --structured --repository=test --signing-service=/pulp/api/v3/signing-services/6be8b4c9-a87a-419c-8b44-95634c0843e1/
Traceback (most recent call last):
File "/home/quirin/.virtualenvs/pulp_cli/bin/pulp", line 8, in <module>
sys.exit(main())
^^^^^^
File "/home/quirin/.virtualenvs/pulp_cli/lib/python3.11/site-packages/pulp_cli/__init__.py", line 31, in main
load_plugins()
File "/home/quirin/.virtualenvs/pulp_cli/lib/python3.11/site-packages/pulp_cli/__init__.py", line 17, in load_plugins
discovered_plugins: Dict[str, ModuleType] = {
^
File "/home/quirin/.virtualenvs/pulp_cli/lib/python3.11/site-packages/pulp_cli/__init__.py", line 18, in <dictcomp>
entry_point.name: entry_point.load()
^^^^^^^^^^^^^^^^^^
File "/home/quirin/.virtualenvs/pulp_cli/lib/python3.11/site-packages/pkg_resources/__init__.py", line 2468, in load
return self.resolve()
^^^^^^^^^^^^^^
File "/home/quirin/.virtualenvs/pulp_cli/lib/python3.11/site-packages/pkg_resources/__init__.py", line 2474, in resolve
module = __import__(self.module_name, fromlist=['__name__'], level=0)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/quirin/.virtualenvs/pulp_cli/lib/python3.11/site-packages/pulpcore/cli/deb/__init__.py", line 6, in <module>
from pulpcore.cli.deb.publication import publication
File "/home/quirin/.virtualenvs/pulp_cli/lib/python3.11/site-packages/pulpcore/cli/deb/publication.py", line 76, in <module>
href_pattern=PulpSigningServiceContext.HREF_PATTERN,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: type object 'PulpSigningServiceContext' has no attribute 'HREF_PATTERN'
Am I doing something wrong, or is the change missing something?
|
More disturbing is the question why the ci workflows did not run. |
|
https://github.com/pulp/pulp-cli/blob/main/pulp-glue/pulp_glue/core/context.py#L341 I don't see that PulpSigningServiceContext ever had an HREF_PATTERN. |
@mdellweg @quba42 I originally added it as part of https://github.com/pulp/pulp-cli/pull/560/files (before glue library was split out) |
|
Created a new pull request for pulp-cli pulp/pulp-cli#652 |
|
@tjmullicani Ok, I can have another go at testing this using both PR's. Not sure if I will find the time today, though. |
hstct
left a comment
hstct
left a comment
There was a problem hiding this comment.
I tested it by adding an href during publication creation and it does work.
LGTM
| default_plugin="deb", | ||
| default_type="apt", | ||
| context_table={"deb:apt": PulpSigningServiceContext}, | ||
| href_pattern=PulpSigningServiceContext.HREF_PATTERN, |
There was a problem hiding this comment.
Since this change is not yet released with pulp-cli, there's a question: Are we ready to bump the minimal required version here already (not until there is a release, i guess), or do we want to fudge the value here for a while with a comment to revert to this final variant once the minimal reqs allow?
There was a problem hiding this comment.
@tjmullicani Can you rebase this branch to latest main branch, and then adjust the following line:
https://github.com/pulp/pulp-cli-deb/blob/main/setup.py#L30
to use "pulp-cli>=0.19.0"?
@mdellweg That is what we need to do here, right? And then we can merge once pulp-cli 0.19.0 has been released?
There was a problem hiding this comment.
Yes, that would work.
Alternatively, something like getattr(PulpSigningServiceContext, "HREF_PATTERN", "/signing_service/") until we really bump that dependency. I just fear that kind of a workaround would possibly be forgotten to remove eventually.
There was a problem hiding this comment.
I have updated the branch.
tjmullicani
force-pushed
the
main
branch
2 times, most recently
from
8028f96 to
afcce36
Compare
tjmullicani
force-pushed
the
main
branch
2 times, most recently
from
baaf693 to
97a5b47
Compare
4 participants
fixes #38