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

Refs #32885: Add puppet user to user_groups only if server or client certificate contains puppet path #938

Merged
merged 1 commit into from
Jun 25, 2021

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Apr 15, 2021

Looking for feedback here as I don't know what the exact correct answer is to prevent the following error when setting puppet::server to false:

2021-04-15 20:08:58 [ERROR ] [configure] Could not set groups on user[foreman]: Execution of '/sbin/usermod -G puppet foreman' returned 6: usermod: group 'puppet' does not exist
2021-04-15 20:08:58 [ERROR ] [configure] /Stage[main]/Foreman::Config/User[foreman]/groups: change from  to 'puppet' failed: Could not set groups on user[foreman]: Execution of '/sbin/usermod -G puppet foreman' returned 6: usermod: group 'puppet' does not exist

It feels like a user of the module should not have to explicitly configure the user_groups if they are setting puppet::server to false and that the module should adjust appropriately.

The puppet-foreman_proxy seems to have a similar failure but different approach: https://github.com/theforeman/puppet-foreman_proxy/blob/941e934a4a27a40e7963bc59f6ab5c14fafb82dc/manifests/config.pp#L33

@ehelms ehelms changed the title Only add puppet user to user_groups if puppet::server is enabled Add puppet user to user_groups if server or client certificate contains puppet path May 18, 2021
manifests/config.pp Outdated Show resolved Hide resolved
spec/acceptance/hieradata/common.yaml Show resolved Hide resolved
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.

I'd like to see some spec test for this. Also, I think the word "only" is missing in the title to indicate the change. Other than that 👍

@ehelms ehelms changed the title Add puppet user to user_groups if server or client certificate contains puppet path Add puppet user to user_groups only if server or client certificate contains puppet path Jun 24, 2021
@ehelms
Copy link
Member Author

ehelms commented Jun 24, 2021

I added a spec test, let me know if you were also wanting an acceptance test.

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.

We should do an installer migration for this as well to remove 'puppet' from the answers file to really benefit from this.

@ehelms ehelms changed the title Add puppet user to user_groups only if server or client certificate contains puppet path Refs #32885: Add puppet user to user_groups only if server or client certificate contains puppet path Jun 25, 2021
ehelms added a commit to ehelms/foreman-installer that referenced this pull request Jun 25, 2021
The change theforeman/puppet-foreman#938
introduced dynamically determining if the puppet group needed to
be added to the foreman user. This no longer needs to be set
explicitly on the foreman::user_groups parameter thus the migration
cleans it up if present.
@ehelms
Copy link
Member Author

ehelms commented Jun 25, 2021

Here is the installer PR -- theforeman/foreman-installer#690

@ehelms ehelms merged commit e16eaa3 into theforeman:master Jun 25, 2021
ehelms added a commit to ehelms/foreman-installer that referenced this pull request Jun 25, 2021
The change theforeman/puppet-foreman#938
introduced dynamically determining if the puppet group needed to
be added to the foreman user. This no longer needs to be set
explicitly on the foreman::user_groups parameter thus the migration
cleans it up if present.
ehelms added a commit to ehelms/foreman-installer that referenced this pull request Jun 25, 2021
The change theforeman/puppet-foreman#938
introduced dynamically determining if the puppet group needed to
be added to the foreman user. This no longer needs to be set
explicitly on the foreman::user_groups parameter thus the migration
cleans it up if present.
ehelms added a commit to ehelms/foreman-installer that referenced this pull request Jun 28, 2021
The change theforeman/puppet-foreman#938
introduced dynamically determining if the puppet group needed to
be added to the foreman user. This no longer needs to be set
explicitly on the foreman::user_groups parameter thus the migration
cleans it up if present.
ehelms added a commit to ehelms/foreman-installer that referenced this pull request Jul 7, 2021
The change theforeman/puppet-foreman#938
introduced dynamically determining if the puppet group needed to
be added to the foreman user. This no longer needs to be set
explicitly on the foreman::user_groups parameter thus the migration
cleans it up if present.
ehelms added a commit to ehelms/foreman-installer that referenced this pull request Jul 12, 2021
The change theforeman/puppet-foreman#938
introduced dynamically determining if the puppet group needed to
be added to the foreman user. This no longer needs to be set
explicitly on the foreman::user_groups parameter thus the migration
cleans it up if present.
ekohl pushed a commit to theforeman/foreman-installer that referenced this pull request Jul 12, 2021
The change theforeman/puppet-foreman#938
introduced dynamically determining if the puppet group needed to
be added to the foreman user. This no longer needs to be set
explicitly on the foreman::user_groups parameter thus the migration
cleans it up if present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants