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

Add openshift registry support for secretless-broker #1141

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

BradleyBoutcher
Copy link
Contributor

  • Added an additional stage for building an openshift container
    compliant image for secretless while making use of the build stage
  • Added a stage to publishing for tagging and submitting the latest
    version of the redhat compliant image
  • Updated our build script to use stage targets, and since both
    stages require different parameters, there would be a failure if
    these target parameters would be left out, rather than overriding
    an image with a different name

bin/publish Outdated
@@ -42,5 +42,17 @@ for image_name in "${IMAGES[@]}"; do
docker tag "$image_name:$FULL_VERSION_TAG" "$REGISTRY/$image_name:$tag"
docker push "$REGISTRY/$image_name:$tag"
done

for tag in "${TAGS[@]}"; do
# Publish only latest to Redhat Registries
Copy link
Contributor

Choose a reason for hiding this comment

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

there is an image with a literal latest tag, so this comment could be clearer (I don't think you mean that we publish only the latest tag to redhat, since you're looping over the TAGS array)

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, I think it's fine if redhat only has the VERSION tag (and not the latest) - I think that's what we do for the authn-k8s client now, and it might be all we can do bc I'm pretty sure RH doesn't let us overwrite tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok that definitely simplifies things! I'll just push the latest version, but not do the whole separate thing for the latest tag. I think openshift actually handles that on its own.

@BradleyBoutcher BradleyBoutcher force-pushed the openshift-image-support branch 8 times, most recently from 9df26d8 to 2c0816f Compare February 11, 2020 19:32
Copy link
Contributor

@izgeri izgeri left a comment

Choose a reason for hiding this comment

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

One last small change and this will be good!

Also, TIL about targets ❇️

bin/build Outdated
# (we want the flags to be word split here)
# shellcheck disable=SC2086
docker build --tag "secretless-broker-redhat:${FULL_VERSION_TAG}" \
--tag "secretless-broker-redhat:latest" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this tag and the comment above be removed?

bin/publish Outdated
Comment on lines 46 to 54
# Publish only latest to Redhat Registries
echo "Tagging and pushing ${REDHAT_IMAGE}"

docker tag "secretless-broker-redhat:${FULL_VERSION_TAG}" "${REDHAT_IMAGE}:${VERSION}"
# you can't push the same tag twice to redhat registry, so ignore errors
if ! docker push "${REDHAT_IMAGE}:${VERSION}"; then
echo 'RedHat push FAILED! (Maybe the image was pushed already?)'
exit 0
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

this section is going to get hit twice, once for image_name secretless-broker and once for image_name secretless-broker-quickstart

looking at it now I think this part needs to be broken out of the image loop and put in its own if [ "$git_description" = "v${VERSION}" ]; then loop

maybe there is a better way to organize it, but it doesn't seem right for it to get hit twice for no reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - fixed!

- Added an additional stage for building an openshift container
compliant image for secretless while making use of the build stage
- Added a stage to publishing for tagging and submitting the latest
version of the redhat compliant image
- Updated our build script to use stage targets, and since both
stages require different parameters, there would be a failure if
these target parameters would be left out, rather than overriding
an image with a different name
Copy link
Contributor

@izgeri izgeri left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @BradleyBoutcher :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants