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

Change sed to not modify /etc/sudoers comments #78

Closed
wants to merge 3 commits into from
Closed

Change sed to not modify /etc/sudoers comments #78

wants to merge 3 commits into from

Conversation

mvermaes
Copy link
Contributor

@mvermaes mvermaes commented Oct 23, 2016

This should fix #79 by only matching non-comment lines.

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@kbsingh
Copy link
Collaborator

kbsingh commented Oct 26, 2016

lgtm, thanks.

@lpancescu thoughts ?

@kbsingh
Copy link
Collaborator

kbsingh commented Oct 26, 2016

ok to test

@lpancescu
Copy link
Contributor

I just extracted the original /etc/sudoers from the sudo packages in CentOS Linux 6 and 7, to take a closer look. There is no requiretty line at all in CentOS Linux 6.8, so I would propose the following:

  • remove the call to sed from centos6.ks;
  • use sed -i 's%^Defaults\s\+requiretty$%# This breaks Vagrant, see https://github.com/mitchellh/vagrant/issues/1482\n# &%' /etc/sudoers in centos7.ks

I see that requiretty was already removed by Red Hat in Fedora 20 (see Fedora bug #1020147). Maybe this is why 6.8 doesn't have it anymore? I'm curious if 7.3 still has that configuration line in /etc/sudoers; if not, we could remove that line from centos7.ks after switching to a 7.3 base.

https://bugzilla.redhat.com/show_bug.cgi?id=1020147

@mvermaes
Copy link
Contributor Author

It looks like it's fixed in sudo-1.8.6p7-18.el7, see https://bugzilla.redhat.com/show_bug.cgi?id=1196451 . I hadn't realized it had already been removed in 6.8.

I've updated to include your suggestions, but feel free to close this request and we can just wait until RHEL 7.3 to see if it can be dropped completely.

@lpancescu
Copy link
Contributor

@mvermaes Thanks! It's just a cosmetic issue, so I'm okay both ways (we could merge it, if we already invested the time - this would also allow us to mark issue #78 as fixed).

With RHEL betas typically taking around two months, plus some additional delay in rebuilding, I guess we might have another 2-3 monthly Vagrant releases until we get the sudo fix.

@mvermaes
Copy link
Contributor Author

I'll leave it up to you then. As you said, it's only cosmetic anyway - it just bothered me that it wasn't quite correct :)

@kenfusion
Copy link

kenfusion commented Nov 9, 2016

Another way is to leave /etc/sudoers alone all together and put this in the /etc/sudoers.d/vagrant file which you already have created:

echo "Defaults:vagrant !requiretty" >> /etc/sudoers.d/vagrant

@mvermaes
Copy link
Contributor Author

Good point, I think that would have been a better way to handle it.

In any case, I tested on RHEL 7.3 and it includes sudo 1.8.6p7-20 which has removed this line per the ticket. Closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sed in Vagrant kickstarts matches extra line
5 participants