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
hack: touch up scripts #953
Conversation
hack/binaries
Outdated
case "$(echo "${TARGETPLATFORM}" | cut -d"/" -f1)" in | ||
"darwin") | ||
case "${TARGETPLATFORM}" in | ||
darwin/*) |
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.
TARGETPLATFORM
can be just darwin
as well. Eg. docker build --platform darwin
works fine and missing sections are added automatically.
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.
oh! right 🤦♂️ should've seen that, I can fix that by changing the case
to;
case "${TARGETPLATFORM%%/*}" in
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.
Fixed 👍
b03da99
to
b10bfb2
Compare
rebased to get rid of the commit from #942 |
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.
Well, I've never seen these scripts before, so my review might not be too helpful.
I found minor issues, though.
hack/images
Outdated
docker build $@ --platform=$PLATFORMS -t $REPO:$TAG . | ||
docker build $@ --platform=$PLATFORMS -t $REPO:$TAG-rootless --target rootless . | ||
docker build $@ --"platform=${PLATFORMS}" -t "${REPO}:${TAG}" . | ||
docker build $@ --"platform=${PLATFORMS}" -t "${REPO}:${TAG}-rootless" --target rootless . |
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.
Placement of quotes looks strange to me. I suggest to either quote the whole thing ("--platform=${PLATFORMS}"
) or just the part that might contain whitespace (--platform="${PLATFORMS}"
).
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.
oh, good catch; I think this might've been a wrong find/replace on my side
hack/images
Outdated
--opt platform=$PLATFORMS \ | ||
--output type=image,name=$REPO:$TAG$tagLatest,$pushFlag | ||
--opt "platform=${PLATFORMS}" \ | ||
--output "type=image,name=${REPO}:${TAG}${TAG}Latest,${PUSH}Flag" |
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.
I think this is wrong.
--output "type=image,name=${REPO}:${TAG}${TAG}Latest,${PUSH}Flag" | |
--output "type=image,name=${REPO}:${TAG}${TAG}Latest,${pushFlag}" |
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.
arf; you're right; looks like I've been sleeping on my latest changes 😓
hack/images
Outdated
--opt platform=$PLATFORMS \ | ||
--output type=image,name=$REPO:$TAG-rootless$tagLatestRootless,$pushFlag | ||
--opt "platform=${PLATFORMS}" \ | ||
--output "type=image,name=${REPO}:${TAG}-rootless${TAG}LatestRootless,${PUSH}Flag" |
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.
same here.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
b10bfb2
to
c3afb58
Compare
updated 🤗 |
docker build --iidfile $iidfile --build-arg BUILDKITD_TAGS="${BUILDKITD_TAGS}" --target buildkit-binaries -f ./hack/dockerfiles/test.Dockerfile --force-rm . | ||
;; | ||
esac | ||
case "${TARGETPLATFORM%%/*}" in |
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.
i find this less readable than cut
needs rebase |
ping @thaJeztah |
Is this still on plan? |
Too many conflicts. Probably can skip the nice to have or opinionated things like full whitespace conversion if you still wish to update this for easier review. |
Created on top of #942 (first commit is from that PR)See individual commits for details