-
Notifications
You must be signed in to change notification settings - Fork 14
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 latest tag #998
Add latest tag #998
Conversation
Reviewer's Guide by SourceryThis pull request addresses an issue with Docker image tagging in the CI pipeline. The main change is the addition of the ':latest' tag when pushing Docker images to the registry. This fix is implemented across multiple CI configuration files to ensure consistent image versioning and accurate end-to-end test results. File-Level Changes
Tips
|
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.
Hey @JoshuaSBrown - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -44,7 +44,7 @@ | |||
- ./scripts/generate_datafed.sh | |||
- docker login "${REGISTRY}" -u "${HARBOR_USER}" -p "${HARBOR_DATAFED_GITLAB_CI_REGISTRY_TOKEN}" | |||
- docker build --build-arg DEPENDENCIES="${REGISTRY}/datafed/dependencies-${BRANCH_LOWER}:latest" --build-arg RUNTIME="${REGISTRY}/datafed/runtime-${BRANCH_LOWER}:latest" -f ${DOCKER_FILE_PATH} -t "${REGISTRY}/${IMAGE_TAG}-${BRANCH_LOWER}:latest" . | |||
- docker push "${REGISTRY}/${IMAGE_TAG}-${BRANCH_LOWER}" |
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.
suggestion: Consider refactoring to reduce duplication across build files
The consistent application of this change across multiple files (build_gcs_base_image.yml, build_gcs_image.yml, force_build_gcs_base_image.yml, force_build_gcs_image.yml, and common.yml) suggests there might be some duplication in the build configuration. In the future, consider refactoring these files to reduce duplication and improve maintainability.
.docker_build_and_push:
script:
- export IMAGE_TAG="${CI_PROJECT_NAME,,}"
- export BRANCH_LOWER="${CI_COMMIT_REF_NAME,,}"
- docker login "${REGISTRY}" -u "${HARBOR_USER}" -p "${HARBOR_DATAFED_GITLAB_CI_REGISTRY_TOKEN}"
- docker build --progress plain -t "${REGISTRY}/${IMAGE_TAG}-${BRANCH_LOWER}:latest" - < "${DOCKERFILE_PATH}"
- docker push "${REGISTRY}/${IMAGE_TAG}-${BRANCH_LOWER}:latest"
@@ -50,6 +50,7 @@ | |||
18. [995] - Fixes issue with project and user folders in repo being created under root user permissions. | |||
19. [994] - Fixes issue with spaces not being preserved in shell scripts from docker compose .env file. | |||
20. [996] - Fixes bug in lego install script where function name had additional s | |||
21. [998] - Fixing missing :latest tag on push in container, in common.yml of ci files |
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.
nitpick (documentation): Consider changing 'Fixing' to 'Fixes' for consistency with other entries.
Description
In installing a DataFed repo at Drexel, it was discovered that there were problems with the image containers. It turns out that the release CI pipelines were missing the :latest tag when pushing to the registry. This was causing the end-to-end tests to give false positives in the pipelines. It is fixed in the devel branch because the CI pipeline files were refactored. Merging this should cause the CI pipelines to reproduce the errors that Drexel and LeHigh were observing.
Summary by Sourcery
Add the :latest tag to Docker image pushes in CI pipeline scripts to ensure the latest version is tagged correctly, and update the changelog to reflect this change.
CI:
Documentation: