Skip to content

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

Merged
AkihiroSuda merged 5 commits intoopencontainers:masterfrom
thaJeztah:debian_buster
Mar 30, 2020
Merged

Dockerfile: some refactoring, and switch to "buster" variant#2234
AkihiroSuda merged 5 commits intoopencontainers:masterfrom
thaJeztah:debian_buster

Conversation

@thaJeztah
Copy link
Copy Markdown
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
Copy Markdown
Member

AkihiroSuda commented Mar 9, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 15, 2020

@cyphar @crosbymichael @kolyshkin LGTY?

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

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

@kolyshkin kolyshkin Mar 15, 2020

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
Copy Markdown
Member Author

@thaJeztah thaJeztah Mar 19, 2020

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
Copy Markdown
Contributor

@kolyshkin kolyshkin Mar 20, 2020

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
Copy Markdown
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
Copy Markdown
Member

AkihiroSuda commented Mar 24, 2020

needs rebase

@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Mar 26, 2020

@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
Copy Markdown
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
Copy Markdown
Member Author

thaJeztah commented Mar 30, 2020

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

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Mar 30, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Mar 30, 2020

CI failure is unrelated and is fixed by #2268

@mrunalp
Copy link
Copy Markdown
Contributor

mrunalp commented Mar 30, 2020 •
edited by AkihiroSuda
Loading

LGTM

Approved with PullApprove

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Mar 30, 2020

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
Copy Markdown
Member Author

thaJeztah commented Mar 30, 2020

Rebased 👍

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Mar 30, 2020 •
edited
Loading

LGTM

Approved with PullApprove

1 similar comment
@mrunalp
Copy link
Copy Markdown
Contributor

mrunalp commented Mar 30, 2020 •
edited by AkihiroSuda
Loading

LGTM

Approved with PullApprove

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Mar 30, 2020

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