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.

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

Change owner while copy #7552

Merged
merged 2 commits into from
Apr 6, 2021
Merged

Change owner while copy #7552

merged 2 commits into from
Apr 6, 2021

Conversation

t3easy
Copy link
Contributor

@t3easy t3easy commented Oct 30, 2019

Copy all files and chown them in a 2nd layer leads to a larger image.
See layer 22 and 26 of https://hub.docker.com/layers/snipe/snipe-it/v4.7.8/images/sha256-67c865d91df1b90cef1112f12bbc9c64402dfeafde0bdb160c4f07e785ee0bcc

@t3easy t3easy requested a review from snipe as a code owner October 30, 2019 23:08
@snipe snipe requested a review from uberbrady February 4, 2020 20:36
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

One small question, but once that's answered it should be good to go! Thanks for the change!

Dockerfile Outdated
@@ -62,8 +62,6 @@ WORKDIR /var/www/html
# COPY docker/*.php /var/www/html/app/config/production/
COPY docker/docker.env /var/www/html/.env
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems great! Would we need to also do it here, though? The later chown seems like it would grab this .env file as well?

Let me know about that (or add it, if needed) and I'll get it approved for you right away!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a commit to the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi there! Would you still be able to make this change?

Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

Excellent; looks great to me! Thank you!

@t3easy
Copy link
Contributor Author

t3easy commented Apr 14, 2020

Excellent; looks great to me! Thank you!

If you like, I could refactor the whole Dockerfile, e.g. use multistage build to get rid of all the composer dependencies (git, svn, ...) in the final image.

@uberbrady
Copy link
Collaborator

I think that would be great, after version 5.0 is released - I'd love to shrink that all down, it'll make everyone much happier.

@t3easy
Copy link
Contributor Author

t3easy commented Apr 18, 2020

I think that would be great, after version 5.0 is released - I'd love to shrink that all down, it'll make everyone much happier.

Cool! After the merge of this PR, I'll look at it.

@snipe snipe merged commit d61d189 into snipe:master Apr 6, 2021
@welcome
Copy link

welcome bot commented Apr 6, 2021

Congrats on merging your first pull request! 🎉🎉🎉

@snipe
Copy link
Owner

snipe commented Apr 6, 2021

CRAAAP. I didn't realize this PR was against master instead of develop. :( :(

@t3easy
Copy link
Contributor Author

t3easy commented Apr 6, 2021

Hi! Should I change something?

@snipe
Copy link
Owner

snipe commented Apr 6, 2021

Nah, it's already merged - I just gotta make sure it doesn't get eaten in the develop -> master merge :)

@t3easy
Copy link
Contributor Author

t3easy commented Apr 6, 2021

Cool! If I find some time, I'll have a look at the Dockerfiles for some more optimizations.

aranar-pro pushed a commit to aranar-pro/snipe-it that referenced this pull request Apr 6, 2021
* Change owner while copy

Copy all files and chown them in a 2nd layer leads to a larger image.
See layer 22 and 26 of https://hub.docker.com/layers/snipe/snipe-it/v4.7.8/images/sha256-67c865d91df1b90cef1112f12bbc9c64402dfeafde0bdb160c4f07e785ee0bcc

* Copy docker.env as user docker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants