Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #30436 - add allowed_export_paths to settings.py #147

Merged
merged 6 commits into from
Dec 2, 2020

Conversation

jeremylenz
Copy link
Contributor

@jeremylenz jeremylenz commented Nov 11, 2020

As part of the new content view import/export API work, we need to be able to add import & export paths to settings.py:

ALLOWED_IMPORT_PATHS = ['/var/lib/pulp/sync_imports', '/var/lib/pulp/imports'] # already exists
ALLOWED_EXPORT_PATHS = ['/var/lib/pulp/exports'] # new

This PR will be modeled on #79 and adds ALLOWED_EXPORT_PATHS.

@jeremylenz

This comment has been minimized.

@jeremylenz jeremylenz marked this pull request as ready for review November 12, 2020 20:16
Copy link
Collaborator

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

Hey @jeremylenz thanks for this PR!

I have a few questions to better understand:

  1. Do you consider the change to default $allowed_import_path to fall under the same Redmine? Should it be split into a separate commit, or separate PR and Redmine entirely?

  2. Should this configuration be the same for standalone pulp3, pulp3 on katello, and pulp3 on content proxy? This module is used for both the katello and foreman_proxy_content installer scenarios and unless the configuration should be the same in both cases, we should expose the new parameter in puppet-foreman_proxy_content so that the installer can pass different values of the parameter per scenario.

@jeremylenz
Copy link
Contributor Author

@wbclark good questions!

For item 1 the change is related and small enough that I think it can stay here in this PR and redmine.
Regarding item 2 - It's definitely relevant for pulp3 on katello. I don't think this is relevant for standalone pulp3, but I'm not sure about pulp3 on content proxy. @parthaa can probably give a better answer.

Copy link
Collaborator

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

OK, thinking about it a bit, content proxies shouldn't need CV import/export since they sync directly from Katello

So I think we should make the following changes:

  1. This PR should include only the new parameter for $allowed_export_path

  2. A second PR should be opened in puppet-FPC which exposes $allowed_{import,export}_path parameters at https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/manifests/init.pp#L371-L394

  3. A 3rd PR should be opened in foreman-installer which overrides the default $allowed_{import,export}_path parameters for the Katello scenario only, and also defines a migration for the Katello scenario that sets the proper values of the parameters for users who already installed

@jeremylenz
Copy link
Contributor Author

@wbclark, Thanks for guiding this newbie 👍

This PR should include only the new parameter for $allowed_export_path

updated

A second PR should be opened in puppet-FPC which exposes $allowed_{import,export}_path parameters at https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/manifests/init.pp#L371-L394

Opened theforeman/puppet-foreman_proxy_content#297

A 3rd PR should be opened in foreman-installer which overrides the default $allowed_{import,export}_path parameters for the Katello scenario only, and also defines a migration for the Katello scenario that sets the proper values of the parameters for users who already installed

Opened theforeman/foreman-installer#615

Let me know how they look..

manifests/init.pp Outdated Show resolved Hide resolved
templates/settings.py.erb Outdated Show resolved Hide resolved
@wbclark
Copy link
Collaborator

wbclark commented Nov 18, 2020

Hey @jeremylenz thanks for the effort here! It's coming together nicely.

I left one comment above about setting defaults that make sense for pulp3 on its own.

The other thing I want to mention, following a similar line of reasoning, is the parameter documentation making references to concepts like Katello and Content Views, which pulpcore itself has no concept of. I think having more neutral documentation (see @ekohl 's suggestion referencing the pulpcore docs) makes sense here, while in puppet-FPC we can document the relevance to Katello users.

I also realize I am saying this that some of those references were already here before your PR (in the ERB...) -- sorry that you inherited that and let us know if we can assist with cleaning it up. :)

@jeremylenz
Copy link
Contributor Author

@wbclark @ehelms @ekohl I tried to address comments on all 3 PR's.. let me know what I missed. thanks!

@jeremylenz
Copy link
Contributor Author

@wbclark thanks, updated!

manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
Comment on lines 366 to 371
it do
is_expected.to compile.with_all_deps
is_expected.to contain_concat__fragment('base')
.with_content(%r{ALLOWED_EXPORT_PATHS = \[\]})

end
Copy link
Member

Choose a reason for hiding this comment

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

This matches the default. I'd move this to the section that starts at line 19. We should probably do the same for import paths.

@jeremylenz
Copy link
Contributor Author

@ekohl updated

@ehelms ehelms merged commit 26ea9a0 into theforeman:master Dec 2, 2020
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Not sure why this was merged. I still had comments about the specs and it was not addressed as I suggested. I also think this should have been squashed and not as 6 separate commits.

@ehelms
Copy link
Member

ehelms commented Dec 2, 2020

Sorry, it looked like your comment had been resolved. I do wish Github would warn me when there are multiple commits, that was my bad.

@ekohl ekohl added the Enhancement New feature or request label Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Needs testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants