-
Notifications
You must be signed in to change notification settings - Fork 545
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
Fix the -R option on Debian/Ubuntu #1082
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super awesome! The code looks DRY and much cleaner now. Great work.
But I would like to offer a couple of suggestions, please take a look below.
bootstrap-salt.sh
Outdated
__REPO_ARCH="$DPKG_ARCHITECTURE" | ||
|
||
if [ "$DPKG_ARCHITECTURE" = "i386" ]; then | ||
echoerror "$_REPO_URL likely doesn't have all required 32-bit packages for Ubuntu $DISTRO_MAJOR_VERSION (yet?)." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we should use ${DISTRO_NAME}
variable instead of hard coded Ubuntu
word here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. Good catch. I will fix that.
bootstrap-salt.sh
Outdated
elif [ "$ITYPE" != "git" ]; then | ||
echoerror "You can try git installation mode, i.e.: sh ${__ScriptName} git v2016.11.5" | ||
fi | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see, this should work properly during stable installation on Debian/Raspbian armhf
, but will output unnecessary error messages which may confuse users.
How about to simplify this whole block to a case
statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
DEBIAN_CODENAME="wheezy" | ||
else | ||
DEBIAN_CODENAME="jessie" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for now. But I guess in the future we would need a better checks for Debian distros codenames, something like we do for Ubuntu above and in the __ubuntu_codename_translation
function.
Winter Debian Stretch is coming...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. I thought of this, too, but I think a separate PR will be better for this right now. I'll make an issue for it, especially since 7 is phasing out and we'll be moving to 9 very soon as you said. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As always, @vutny added valuable input!
@vutny I have updated this PR with your suggestions. Please take another look. |
bootstrap-salt.sh
Outdated
error_msg="" | ||
;; | ||
"armhf") | ||
error_msg="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are no armhf
packages for Ubuntu and Debian 7, maybe we should also check that here?
@rallytime That looks really great. Just added some another suggestion above. I know I'm bothersome 😃 |
@vutny Don't worry, this is good feedback! My latest change adds warnings for when the architecture detected is Let me know what you think. |
Go Go Jenkins! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid!
Go Go Jenkins! |
@s0undt3ch I am not sure why the clone job is failing on these tests. I am not as familiar with this jenkins set up. Do you know why that |
And remove legacy support for pre-repo.saltstack.com packages for Ubuntu. This change also fixes the issue where Ubuntu installations using the -R option were not working.
This function combines the previous efforts to install the correct saltstack repo for debian 7 and debian 8 into a single codeblock. This change also fixes the issue where Debian installations using the -R option were not working.
- Added checks and warnings for armhf packages not supported on Ubuntu/Debian 7 - Moved __check_dpkg_architecture below __ubuntu_derivatives_translation function to use the "DISTRO_NAME_L" value - Added return codes to various warning cases since if we detect an unsupported architecture, we should fail sooner.
What does this PR do?
When the
-R
option was originally implemented for Debian/Ubuntu, I must not have been testing the right thing because I am not sure how that ever could have worked. This PR reworks some of those-R
checks and sets up the repositories correctly.What issues does this PR fix or reference?
Fixes #1081
Previous Behavior
The
-R
option wouldn't set up any repo, and would install the default old packages in apt, which is version 0.17. :(New Behavior
-R option works well!
I also did a bunch of clean up in this PR that I have been wanting to do for a while. Namely:
install_saltstack_ubuntu_repository
function and moved the saltstack repo set up (and also will work with the-R
setting with the_REPO_URL
value) there. This is similar to the way the RHEL distros gate and set up the saltstack repo.install_saltstack_debian_repository
function and moved the saltstack repo set up for BOTH debian 7 and debian 8 into one function. This also works with-R
, just like the ubuntu repo set up function.