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 #32309 - pulpcore-manager fails from certain directories #181

Merged

Conversation

ianballou
Copy link
Contributor

Sets a specific directory for pulpcore-manager to be called from.

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.

The change itself makes sense to me. However, please make the commit message more descriptive. Like Always run admin cmds from Pulp's home (not sure how much will fit). Ideally with then a body that describes that the cwd might be an unreadable directory like /root and dynaconf appears to unconditionally read the current directory. Pulp's user home should always be readable for the user pulp.

Also, it might be good to link to a dynaconf bugreport. IMHO it shouldn't error out on unreadable CWD.

@@ -32,11 +35,13 @@
String $user = $pulpcore::user,
Stdlib::Absolutepath $pulp_settings = $pulpcore::settings_file,
Optional[Integer[0]] $timeout = undef,
String $working_dir = $pulpcore::user_home,
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to be stricter, even if it doesn't really make a difference.

Suggested change
String $working_dir = $pulpcore::user_home,
Stdlib::Absolutepath $working_dir = $pulpcore::user_home,

@ianballou ianballou force-pushed the 32309-change-dir-before-pulpcore-manager branch from aec9bcb to c56861f Compare April 13, 2021 18:26
@ianballou ianballou marked this pull request as ready for review April 13, 2021 18:41
@ianballou
Copy link
Contributor Author

Not sure yet why the tests are complaining about Unknown variable: 'pulpcore::user_home'.

@ekohl
Copy link
Member

ekohl commented Apr 13, 2021

In that testcase we don't include the class pulpcore so it can't reference the variable. That's why we explicitly set those in the tests:

context 'with a fixed pulp_settings and user' do
let(:params) do
{
pulp_settings: '/etc/pulpcore/settings.py',
user: 'pulpcore',
}
end

@ianballou ianballou force-pushed the 32309-change-dir-before-pulpcore-manager branch from c56861f to 2bf597e Compare April 13, 2021 19:11
@ianballou
Copy link
Contributor Author

@ekohl I think we're all good now.

@ianballou ianballou requested a review from ekohl April 13, 2021 20:16
@@ -11,6 +11,7 @@
{
pulp_settings: '/etc/pulpcore/settings.py',
user: 'pulpcore',
working_dir: '/var/lib/pulp'
Copy link
Collaborator

@wbclark wbclark Apr 13, 2021

Choose a reason for hiding this comment

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

Hey @ianballou , could you please add to the test below, under is_expected.to contain_exec('pulpcore-manager help'),

.with_cwd('/var/lib/pulp')

This way we are not only passing the param from the parent module which pulpcore::admin needs, we are also testing that the feature works properly at the business logic level (puppet catalogue) and continues working as expected with any future changes.

* Dynaconf seems to always be using the current directory. As such, the
  cwd could be a directory that is unreadable by the pulp user.  Pulp's
  user home must always be readable by the pulp user.
@ianballou ianballou force-pushed the 32309-change-dir-before-pulpcore-manager branch from 2bf597e to bf40b19 Compare April 13, 2021 22:16
@ianballou
Copy link
Contributor Author

@wbclark okay, I added the test condition.

@wbclark
Copy link
Collaborator

wbclark commented Apr 13, 2021

@wbclark okay, I added the test condition.

Great, thank you. This looks good to me. Let's see what @ekohl says

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.

This is what I would have written as well

@ekohl ekohl added the Bug Something isn't working label Apr 14, 2021
@ekohl ekohl merged commit 951a2f8 into theforeman:master Apr 14, 2021
@ianballou ianballou deleted the 32309-change-dir-before-pulpcore-manager branch April 14, 2021 14:58
@goosemania
Copy link

The change itself makes sense to me. However, please make the commit message more descriptive. Like Always run admin cmds from Pulp's home (not sure how much will fit). Ideally with then a body that describes that the cwd might be an unreadable directory like /root and dynaconf appears to unconditionally read the current directory. Pulp's user home should always be readable for the user pulp.

Also, it might be good to link to a dynaconf bugreport. IMHO it shouldn't error out on unreadable CWD.

FWIW, tried to fix, this helped https://github.com/rochacbruno/dynaconf/pull/570

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Needs testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants