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

Automatically enable EPEL after prompting users #3259

Merged
merged 3 commits into from Aug 18, 2016
Merged

Automatically enable EPEL after prompting users #3259

merged 3 commits into from Aug 18, 2016

Conversation

bmw
Copy link
Member

@bmw bmw commented Jul 8, 2016

Fixes #2158.

I tested all branches of this on CentOS 6 and tested the script on CentOS 7 (EPEL doesn't need to be enabled on CentOS 7 so there's not much to test). I also tested this on the test farm, however, it appears EPEL is already enabled so I think the value of testing there is fairly minimal.

@bmw bmw added this to the 0.9.0 milestone Jul 8, 2016
@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage remained the same at 98.766% when pulling ae1a7af on epel-auto into 031b41a on master.

@pde
Copy link
Member

pde commented Jul 8, 2016

@NoodlesNZ or @hogarthj would either of you (or someone else from your team) be interested in reviewing this? This is realistically going to be the way that CentOS 6 users get to install Certbot for the rest of time...

@bmw
Copy link
Member Author

bmw commented Jul 8, 2016

Just noticed that I had some changes locally that weren't included here. I've updated this PR.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.766% when pulling 44113a5 on epel-auto into 031b41a on master.

echo "To use Certbot, packages from the EPEL repository need to be installed."
if [ "$ASSUME_YES" = 1 ]; then
/bin/echo -n "Enabling the EPEL repository in 3 seconds..."
sleep 1s
Copy link
Member

@erikrose erikrose Jul 27, 2016

Choose a reason for hiding this comment

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

Is the s an actual thing? I can't find it documented in the Fedora man page, and OS X, at least, just ignores the s. You can sleep 1q with equal results.

Copy link
Member

Choose a reason for hiding this comment

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

sleep on Fedora etc is GNU sleep, which supports the s (as well as m and h).

@erikrose
Copy link
Member

That's all I see. Otherwise, it looks fine.

@pde pde assigned bmw and unassigned erikrose Aug 3, 2016
@pde
Copy link
Member

pde commented Aug 3, 2016

@bmw I think this is back in your court to consider enabling the "optional" channel for RHN users.

@pde pde changed the title Automatically enable EPEL after prompting users Automatically enable EPEL after prompting users [revision requested] Aug 3, 2016
@pde
Copy link
Member

pde commented Aug 17, 2016

@bmw it may be that we can kick the RHN piece into a future ticket. If you agree, go ahead and merge this!

@bmw
Copy link
Member Author

bmw commented Aug 17, 2016

I'm really struggling testing this on a Red Hat machine with RHN. Does anyone else want to take a crack at it?

@bmw bmw removed their assignment Aug 17, 2016
@bmw
Copy link
Member Author

bmw commented Aug 18, 2016

So it appears that you can only yum install epel-release on CentOS. RHEL users are told to wget a package and install it directly through rpm.

Due to this oversight on my part, we will error out on RHEL systems (or CentOS systems that don't include the extras repository which is enabled by default) for now with a message asking the user to enable EPEL. This is a better user experience anyway than erroring out later with an error message like virtualenv: command not found. I created #3433 to track the issue of automatically enabling EPEL on RHEL.

@bmw bmw changed the title Automatically enable EPEL after prompting users [revision requested] Automatically enable EPEL after prompting users Aug 18, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 98.76% when pulling 156c641 on epel-auto into 031b41a on master.

@@ -287,6 +287,10 @@ BootstrapRpmCommon() {

if ! $SUDO $tool list *virtualenv >/dev/null 2>&1; then
echo "To use Certbot, packages from the EPEL repository need to be installed."
if ! $SUDO $tool list epel-release >/dev/null 2>&1; then
Copy link
Member

Choose a reason for hiding this comment

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

As long as we're sure that this package will always be visible on RPM-based systems where certbot-auto works.

Copy link
Member

Choose a reason for hiding this comment

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

If we're not, I guess we can do something like make this a warning?

Copy link
Member

Choose a reason for hiding this comment

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

Actually I guess this is fine, because we're inside the if about virtualenv. So overall, LGTM.

@pde pde merged commit 387d61d into master Aug 18, 2016
@bmw bmw deleted the epel-auto branch August 18, 2016 22:28
@pde pde removed the has pr label Oct 6, 2016
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.

None yet

4 participants