-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add initial jq-based templating engine #730
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
e0ac642
to
ca92432
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, nice work!
Just a few (minor) comments. 👍
apply-templates.sh
Outdated
for version; do | ||
export version | ||
|
||
if [ -n "$(jq -r '.[env.version].debian // ""' versions.json)" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this jq
fails for some reason (syntax error, .[env.version]
being undefined, etc), then this script will silently skip this $version
instead of also failing, so I think we need to split this out:
if [ -n "$(jq -r '.[env.version].debian // ""' versions.json)" ]; then | |
debianVersion="$(jq -r '.[env.version].debian // ""' versions.json)" | |
if [ -n "$debianVersion" ]; then |
Alternatively, we could (should?) ditch this conditional completely and just let the template generate always -- if we someday remove/deprecate the "debian" variant in a future major/minor release post-#680, then we would introduce a check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the if
was inspired by a similar section in 680: https://github.com/docker-library/mysql/pull/680/files#diff-e14594f6bb9b29ac0146c04e78ae33e8e92f2f00709fefab97359dea31b94d11R98-R127. It seems fine to keep.
(I look forward to rebasing #680 😆) |
ca92432
to
7f0d6c8
Compare
7f0d6c8
to
ee33a21
Compare
Looks great -- actual |
Changes: - docker-library/mysql@79fb37b: Merge pull request docker-library/mysql#730 from infosiftr/jq-template - docker-library/mysql@ee33a21: Add initial jq-based templating engine
Similar to docker-library/php#1052.
This also takes some bits (like
dirCommit
andDockerfile.debian
) from #680 in order to support that PR easier.The new Dockerfile template should make it easier to keep the version-specific Dockerfiles in sync.