-
Notifications
You must be signed in to change notification settings - Fork 160
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
Improve tools/install_docker #2125
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.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kormat)
tools/install_docker, line 8 at r1 (raw file):
# Install prereqs DEBIAN_FRONTEND=noninteractive apt-get install apt-transport-https ca-certificates curl lsb-release software-properties-common -y
I prefer flags before arguments, so maybe move -y to just after apt-get
or install
?
tools/install_docker, line 11 at r1 (raw file):
chksum() { echo "$sum $file" | sha256sum --status -c -
This is a bit ugly in that it uses global state instead of parameters.
tools/install_docker, line 45 at r1 (raw file):
# Install docker-ce DEBIAN_FRONTEND=noninteractive apt-get install docker-ce=18.06* -y
Same as for line 8 (move -y
)
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.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @klausman)
tools/install_docker, line 8 at r1 (raw file):
Previously, klausman (Tobias Klausmann) wrote…
I prefer flags before arguments, so maybe move -y to just after
apt-get
orinstall
?
Done.
tools/install_docker, line 11 at r1 (raw file):
Previously, klausman (Tobias Klausmann) wrote…
This is a bit ugly in that it uses global state instead of parameters.
Done.
tools/install_docker, line 45 at r1 (raw file):
Previously, klausman (Tobias Klausmann) wrote…
Same as for line 8 (move
-y
)
Done.
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.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Reviewed 1 of 2 files at r3.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @klausman)
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.
Reviewed 1 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved
- Ensure it is run as root. - Make it idempotent. - Support linuxmint as well as plain ubuntu. - Use non-interactive frontend for apt-get. - Don't hard-code amd64.
This change is