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

generate-stackbrew-library.sh: Fix detection of deleted files in COPY command #805

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

teohhanhui
Copy link

@teohhanhui teohhanhui commented Mar 28, 2019

Don't let the shell do glob expansion, because it would not find deleted files. git can.

(Discovered when I did coopTilleuls/docker-varnish#39, and fixed in coopTilleuls/docker-varnish#40)

/cc @tianon

References:
https://github.com/koalaman/shellcheck/wiki/SC2207
https://github.com/koalaman/shellcheck/wiki/SC2068

@yosifkit
Copy link
Member

We don't really have the problem of files deleted without some other change in the Dockerfile context directory. php is the only one we maintain with a glob in the COPY/ADD and I doubt we'll be deleting any of the docker-php-ext- scripts anytime soon.

@teohhanhui
Copy link
Author

teohhanhui commented Mar 28, 2019

Well, I don't see any down side to this change. Do you? :)

It makes the script more robust and removes a shellcheck warning.

@teohhanhui
Copy link
Author

teohhanhui commented Mar 28, 2019

Anyway, I think it might be very useful for Alpine variants, where patches may be routinely required (because not everybody cares so much about musl, unfortunately).

@yosifkit
Copy link
Member

The downside is the extra initial churn to PRing and/or committing this to our other 30+ repos in order to keep their generate-stackbrew functions in sync.

As for shellcheck warnings, they are irrelevant. We don't use shellcheck.

where patches may be routinely required.

If there is an obsolete patch, then the next version bump would fail to build and thus necessitate that we remove the patch in order to bump the version (ie "some other change in the Dockerfile context directory"). We don't commit a version bump without a successful build and test. On the other hand, if it builds successfully with a obsolete patch, then the patch is harmless and can be removed at the next context change (version bump).

@teohhanhui
Copy link
Author

The downside is the extra initial churn to PRing and/or committing this to our other 30+ repos in order to keep their generate-stackbrew functions in sync.

Happy to wait until some other necessary change comes along, of course. :)

As for shellcheck warnings, they are irrelevant. We don't use shellcheck.

Shellcheck helps us write better shell scripts. Of course, you don't have to use shellcheck, but it's a valid warning in this case.

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.

2 participants