Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

Rebase be39b6d - select yum or apt-get in systemd-resolved #228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tonyhb
Copy link

@tonyhb tonyhb commented Jun 11, 2021

Rebase of #204 for tests.

@tonyhb tonyhb requested a review from robmorgan as a code owner June 11, 2021 17:48
@hashicorp-cla
Copy link

hashicorp-cla commented Jun 11, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Code changes LGTM, except for a couple tiny NITs. Once those are fixed, I can kick off tests.

@@ -67,14 +67,30 @@ function assert_not_empty {
fi
}

function has_yum {
[ -n "$(command -v yum)" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: we follow the Google Bash style guide, so would you mind updating to [[ ]] instead of [ ]?

echo iptables-persistent iptables-persistent/autosave_v6 boolean true | sudo debconf-set-selections
sudo apt-get install -y iptables-persistent
if has_apt_get; then
sudo apt-get update -y
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: Not related to this PR, but I think we want:

Suggested change
sudo apt-get update -y
sudo DEBIAN_FRONTEND=noninteractive apt-get update -y

Without it, we may occasionally get an interactive prompt here, causing the build to hang on input.

sudo apt-get update -y
echo iptables-persistent iptables-persistent/autosave_v4 boolean true | sudo debconf-set-selections
echo iptables-persistent iptables-persistent/autosave_v6 boolean true | sudo debconf-set-selections
sudo apt-get install -y iptables-persistent
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: Not related to this PR, but I think we want:

Suggested change
sudo apt-get install -y iptables-persistent
sudo DEBIAN_FRONTEND=noninteractive apt-get install -y iptables-persistent

Without it, we may occasionally get an interactive prompt here, causing the build to hang on input.

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

Successfully merging this pull request may close these issues.

3 participants