Skip to content

Add Dockerfile #5169

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
Merged
sjackman merged 1 commit into from
Oct 25, 2018
Merged

Add Dockerfile #5169

sjackman merged 1 commit into from
Oct 25, 2018

Conversation

@sjackman
Copy link
Contributor

@sjackman sjackman commented Oct 24, 2018

No description provided.

@sjackman sjackman self-assigned this Oct 24, 2018
@sjackman sjackman requested a review from MikeMcQuaid October 24, 2018 04:37
scpeters
scpeters reviewed Oct 24, 2018
View reviewed changes
MikeMcQuaid
MikeMcQuaid reviewed Oct 24, 2018
View reviewed changes
Dockerfile Outdated
Copy link
Member

@MikeMcQuaid MikeMcQuaid Oct 24, 2018

Choose a reason for hiding this comment

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

What's this for? If cleaning: is there an apt command that can be used instead?

Copy link
Contributor

@scpeters scpeters Oct 24, 2018

Choose a reason for hiding this comment

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

This is what they recommend as a best practice in the docker documentation. I'm not sure if there's an apt command that can be used instead; I've always seen it like this.

Copy link
Contributor Author

@sjackman sjackman Oct 24, 2018

Choose a reason for hiding this comment

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

I would have expected apt-get clean to have an option to remove /var/lib/apt/lists, but I was not able to find such an option. See https://linux.die.net/man/8/apt-get

Dockerfile Outdated
Copy link
Member

@MikeMcQuaid MikeMcQuaid Oct 24, 2018

Choose a reason for hiding this comment

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

Is this standard practise for Docker?

Copy link
Contributor

@scpeters scpeters Oct 24, 2018

Choose a reason for hiding this comment

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

I didn't see it recommended on the official docker website, but I see something similar here:

Copy link
Contributor Author

@sjackman sjackman Oct 24, 2018

Choose a reason for hiding this comment

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

Standard practice for Docker is to leave the user as root, but brew refuses to run as root.

Minor style point, I can instead put this file in /etc/sudoers.d/. Any preference?

Copy link
Contributor

@scpeters scpeters Oct 25, 2018

Choose a reason for hiding this comment

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

Minor style point, I can instead put this file in /etc/sudoers.d/. Any preference?

No preference from me; I defer.

@chdiza
Copy link
Contributor

chdiza commented Oct 24, 2018

I hope that this can instead be named .Dockerfile, so that users don't have this cluttering up the dir.

Same for the recently added azure thingy; can it be .azure-piplelines.yml instead?

The other CI/auxiliary support stuff in the root dir are all dotfiles: .travis.yml, .yardopts, .github, .editorconfig.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Oct 24, 2018

I hope that this can instead be named .Dockerfile, so that users don't have this cluttering up the dir.

Same for the recently added azure thingy; can it be .azure-piplelines.yml instead?

We're using the default names for both because almost all tooling expects them to have those names. Now that this stuff isn't in the /usr/local root there's not a strong argument to me for needing to hide things away in this repository compared to a normal one.

scpeters
scpeters approved these changes Oct 25, 2018
View reviewed changes
Copy link
Contributor

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

brew doctor is clean now; nice work

@sjackman
Copy link
Contributor Author

sjackman commented Oct 25, 2018

brew tests and brew test-bot will be clean after PR #5180.
@scpeters Care to review this PR for me? I'll merge this PR after that one, and then set up Docker Hub to build the image on each commit.

@scpeters
Copy link
Contributor

scpeters commented Oct 25, 2018

Sure, I'll review it in between meetings today.

@sjackman sjackman merged commit b0e3e73 into Homebrew:master Oct 25, 2018
@sjackman sjackman deleted the dockerfile branch October 25, 2018 21:17
@sjackman
Copy link
Contributor Author

sjackman commented Oct 25, 2018

Merged! I'll set up a Docker Hub build now.

@sjackman
Copy link
Contributor Author

sjackman commented Oct 25, 2018

Done. See https://hub.docker.com/r/linuxbrew/brew/builds/

@scpeters
Copy link
Contributor

scpeters commented Oct 26, 2018

nice work! I was just experimenting with it and noticed a problem with the python@2 formula. Should I report that as an issue at https://github.com/Linuxbrew/homebrew-core/issues ?

@sjackman
Copy link
Contributor Author

sjackman commented Oct 26, 2018

Yes, please!

@lock lock bot added the outdated PR was locked due to age label Nov 25, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

+3 more reviewers

@colindean colindean colindean approved these changes

@scpeters scpeters scpeters approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sjackman sjackman

Labels

outdated PR was locked due to age

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sjackman @chdiza @MikeMcQuaid @scpeters @colindean