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.

Sign up for GitHub

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

Jump to bottom

Dockerfile: some refactoring, and switch to "buster" variant #2234

Merged
merged 5 commits into from
Mar 30, 2020
Merged

Dockerfile: some refactoring, and switch to "buster" variant #2234

merged 5 commits into from
Mar 30, 2020

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 23, 2020 •
edited
Loading

Debian buster is the current stable, so might be good to switch to that version; also did some refactoring of the dockerfile (see individual commits for details).

I'll open a follow-up PR to update to go 1.13, but thought to first make the switch to "buster" to keep changes smaller.

@thaJeztah thaJeztah marked this pull request as ready for review February 24, 2020 01:17
@thaJeztah thaJeztah mentioned this pull request Feb 28, 2020
Closed
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 9, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@thaJeztah
Copy link
Member Author

@cyphar @crosbymichael @kolyshkin LGTY?

kolyshkin
kolyshkin reviewed Mar 15, 2020
View reviewed changes
Dockerfile Outdated
&& apt-get clean
&& dpkg --add-architecture ppc64el

RUN apt-get update && apt-get install -y --no-install-recommends \
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for splitting a single RUN statement into two?

Copy link
Member Author

Choose a reason for hiding this comment

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

The dpkg --add-architecture does not have to be re-run when we do apt-get update, and I thought it was slightly cleaner, but let me know if you feel strongly about that

Copy link
Contributor

Choose a reason for hiding this comment

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

I like a single RUN slightly better, but I'm fine either way

@kolyshkin kolyshkin mentioned this pull request Mar 15, 2020
Merged
2 tasks
kolyshkin
kolyshkin approved these changes Mar 20, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin kolyshkin mentioned this pull request Mar 23, 2020
Merged
@AkihiroSuda
Copy link
Member

needs rebase

@kolyshkin
Copy link
Contributor

@thaJeztah could you please rebase it?

@mrunalp can we merge it? It is needed to unblock #2239 which, in turn, is needed to unblock a few PRs that require golang 1.13

@mrunalp
Copy link
Contributor

mrunalp commented Mar 27, 2020

Yeah, but it needs a rebase now :/

@thaJeztah thaJeztah force-pushed the debian_buster branch 2 times, most recently from 14dd443 to 1351fa1 Compare March 30, 2020 09:05
@thaJeztah
Copy link
Member Author

@kolyshkin @mrunalp @AkihiroSuda sorry for the delay; rebased and combined the two RUN's again while at it

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 30, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@kolyshkin
Copy link
Contributor

CI failure is unrelated and is fixed by #2268

@mrunalp
Copy link
Contributor

mrunalp commented Mar 30, 2020 •
edited by AkihiroSuda
Loading

LGTM

Approved with PullApprove

@crosbymichael
Copy link
Member

can you rebase for the CI fix?

@thaJeztah
Dockerfile: use build-args to allow overriding versions
201152a 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Dockerfile: set DEBIAN_FRONTEND=noninteractive
c4821c2 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Dockerfile: sort dependencies, and cleanup apt cache
a596387 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Dockerfile: prevent busting build-cache for busybox rootfs
48bf88c 
Move adding the source code to the end, so that the busybox rootfs
doesn't have to be fetched again on each code change.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Dockerfile: switch to "buster" variant (current stable)
dca34a0 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

Rebased 👍

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 30, 2020 •
edited
Loading

LGTM

Approved with PullApprove

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented Mar 30, 2020 •
edited by AkihiroSuda
Loading

LGTM

Approved with PullApprove

@AkihiroSuda
Copy link
Member

Flaky failure in systemd test seems unrelated. Merging.

$ travis_setup_go
gimme: given '1.13.x' but no release for '1.13' found
gimme: given '1.13.x' but no release for '1.13' found
Failed to run gimme
The command "travis_setup_go" failed and exited with 86 during .

https://travis-ci.org/github/opencontainers/runc/jobs/668927361?utm_medium=notification&utm_source=github_status

@AkihiroSuda AkihiroSuda merged commit 4a9e174 into opencontainers:master Mar 30, 2020
@thaJeztah thaJeztah deleted the debian_buster branch March 30, 2020 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

Assignees
No one assigned
Labels
easy-to-review enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thaJeztah @AkihiroSuda @kolyshkin @mrunalp @crosbymichael