Conversation
| action: append_unique | ||
| type: FQDN | ||
| parameter: --certificate-cname | ||
| pulp_extra_import_paths: |
There was a problem hiding this comment.
The user doesn't care that it's Pulp right? How about:
| pulp_extra_import_paths: | |
| content_extra_import_paths: |
What makes them "extra" ?
There was a problem hiding this comment.
Good point. I used extra because users can add extra or additional import/export paths. Would you prefer something other than extra?
There was a problem hiding this comment.
I'm trying to remember, I think the installer uses this "extra" terminology.
There was a problem hiding this comment.
Ahh, it uses additional. So there are the "you can use these out of the box paths" and then there are "user has some setup, they want to use these paths and accept the requirements to make this work".
| - `pulp_database_ssl_mode`: Database SSL mode (default: `disabled`) | ||
| - `pulp_database_ssl_ca`: Path to the database SSL CA certificate on the control node (default: empty) | ||
|
|
||
| Usage Inside foremanctl |
There was a problem hiding this comment.
I am not sure this detail makes sense at this level. I'd rather see a section in the deployment document around content or add a content document.
|
|
||
| Variables | ||
| --------- | ||
| - `pulp_container_image`: Container image to use for Pulp (default: `quay.io/foreman/pulp`) |
There was a problem hiding this comment.
I am wary of storing defaults in the documentation here. The code can show those and reduce our overhead.
There was a problem hiding this comment.
I was hoping someone would comment on the docs. I tried to follow similar patterns from the roles/hammer/README.md. I also feel this is kinda an info dump but wanted to get opinions on it. I don't mind removing this doc from the PR if it makes more sense not to have this
There was a problem hiding this comment.
I like the having of a README. I can see now that this is following roughly the outline from the hammer role. I think as long as we are consistent in the structure then keep it.
There was a problem hiding this comment.
And we should define that roughly in https://github.com/theforeman/foremanctl/blob/master/docs/developer/playbooks-and-roles.md#shared-metadata-fragments
You do not have to do that here in this PR. I am noting this as a reminder.
| Pulp Role | ||
| ========= | ||
|
|
||
| Deploys and manages [Pulp](https://pulpproject.org/) as Podman containers. |
There was a problem hiding this comment.
| Deploys and manages [Pulp](https://pulpproject.org/) as Podman containers. | |
| Deploys and manages [Pulp](https://pulpproject.org/) as Podman quadlets. |
| - imports | ||
| - exports | ||
|
|
||
| - name: Create extra import directories |
There was a problem hiding this comment.
I assume these would have to be mounted into the container?
There was a problem hiding this comment.
Yeah I didn't think about that, I will add this
| PULP_ALLOWED_IMPORT_PATHS: >- | ||
| {{ pulp_default_import_path + pulp_extra_import_paths }} | ||
| PULP_ALLOWED_EXPORT_PATHS: >- | ||
| {{ pulp_default_export_path + pulp_extra_export_paths }} |
There was a problem hiding this comment.
Locally I have this:
PULP_ALLOWED_IMPORT_PATHS: >-
['/var/lib/pulp/sync_imports', '/var/lib/pulp/imports']
PULP_ALLOWED_EXPORT_PATHS: >-
['/var/lib/pulp/exports']
These would be the default paths..
|
|
||
| pulp_enable_analytics: false | ||
|
|
||
| pulp_default_import_path: |
There was a problem hiding this comment.
This says import_path but it's an array.
I wonder can we have a single variable for import and one for export, and they are just lists that appended to?
There was a problem hiding this comment.
I think this is a good idea. So instead of 4 variables (2 default, 2 lists to hold user passed values). We could just have 2 lists both by default containing the default path and just append user passed values to these lists.
Am I understanding this correctly?
|
Can we make these import/export arguments also available via |
Consider this method for defining them -- https://github.com/theforeman/foremanctl/blob/master/docs/developer/playbooks-and-roles.md#shared-metadata-fragments |
|
I just tried this out, seeing the following in my pulp-api service definition: So far so good, assuming the escaping is correct. And yes, I like to be able to import from my export directory. |
|
Export and import worked! I was able to generate |
|
@ehelms I am not too sure on how to make sure the created import and export directories are mounted into the container. Can you point me into the right direction. Maybe somewhere in the code we handle a similar workflow |
You'll need to do something like this -- https://github.com/theforeman/foremanctl/blob/master/src/roles/pulp/tasks/main.yaml#L137 |
|
|
||
| pulp_enable_analytics: false | ||
|
|
||
| # pulp import/export paths |
| pulp_volumes: >- | ||
| {{ | ||
| ['/var/lib/pulp:/var/lib/pulp'] + | ||
| (pulp_import_paths | map('regex_replace', '^(.+)$', '\1:\1') | list) + |
There was a problem hiding this comment.
lets take one item from pulp_import_paths and call it string1, so string1="/foo/bar"
this regex turns string1 into the same format as the line above. ('/var/lib/pulp:/var/lib/pulp').
So after string1 runs through regex, we get "/foo/bar:/foo:bar"
| foreman_development_github_username: | ||
| help: GitHub username to add as additional remote for git checkouts | ||
| action: store | ||
| pulp_import_paths: |
There was a problem hiding this comment.
You could define these in a file like https://github.com/theforeman/foremanctl/blob/master/src/playbooks/_database_connection/metadata.obsah.yaml and then include that file in both deploy and deploy-dev.
Why are you introducing these changes? (Problem description, related links)
Jira
Add pulp content import/export arguments.
What are the changes introduced in this pull request?
You can pass in multiple args of either
--content-import-pathor--content-export-pathas many times as you want, since these will be appended as a list. These folders will be automatically created.How to test this pull request
pytest tests/pulp_test.pyCan also manually test these steps.