-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
chore: cleaning up final references to slim from skaffold #9616
chore: cleaning up final references to slim from skaffold #9616
Conversation
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.
Hi team,
I'm currently reviewing this pull request and will have a full review up in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:
This PR, by Darien-Lin, aims to clean up remaining references to 'slim' within the Skaffold hack
directory. Specifically, the change modifies the update-lts-dependencies.sh
script. The script iterates through Dockerfiles, updating environment variables. This PR removes the Dockerfile.deps.slim
entry from the loop, effectively removing the processing of this specific Dockerfile.
The intent is likely to simplify the build process and remove any unnecessary dependencies related to the 'slim' variant, perhaps due to its deprecation or removal. The change is localized to a single script and appears straightforward.
And now, a little haiku to lighten the mood:
Slim's shadow departs,
Clean code shines, a brighter day,
Skaffold builds anew.
Thanks,
[Your Name]
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.
This pull request removes the final references to slim
in the hack/update-lts-dependencies.sh
script. This change seems aligned with the intent described in the pull request title and description. Overall, the change is small and straightforward. However, I have a few suggestions for improvement.
@@ -114,7 +114,7 @@ For gcloud there is no api endpoint to check for the latest version so the outpu | |||
the machine running this command has the latest gcloud version. Manual modification of the script is required in cases where \ | |||
the version found by the script is incorrect." | |||
|
|||
for dockerfile in "Dockerfile.deps" "Dockerfile.deps.lts" "Dockerfile.deps.slim"; do | |||
for dockerfile in "Dockerfile.deps" "Dockerfile.deps.lts"; do |
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.
Since Dockerfile.deps.slim
is being removed, could you please add a comment explaining why this file is no longer needed? This will help future maintainers understand the reasoning behind this removal and prevent potential confusion or accidental reintroduction. For example, you could mention if the slim
variant is deprecated or if it's being replaced by another approach.
for dockerfile in "Dockerfile.deps" "Dockerfile.deps.lts"; do | |
for dockerfile in "Dockerfile.deps" "Dockerfile.deps.lts"; do # Removed Dockerfile.deps.slim as it's no longer needed (reason) |
sed -i "s/ENV KUBECTL_VERSION .*/ENV KUBECTL_VERSION ${KUBECTL_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile} | ||
sed -i "s/ENV HELM_VERSION .*/ENV HELM_VERSION ${HELM_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile} | ||
sed -i "s/ENV KUSTOMIZE_VERSION .*/ENV KUSTOMIZE_VERSION ${KUSTOMIZE_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile} |
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.
Now that Dockerfile.deps.slim
is removed, it's a good opportunity to review if these sed
commands are still optimal. Since the loop now only iterates over two files instead of three, it might be simpler and more readable to replace the loop with two separate sed
commands, one for each Dockerfile. This avoids the loop overhead and makes the script slightly easier to understand at a glance.
sed -i "s/ENV KUBECTL_VERSION .*/ENV KUBECTL_VERSION ${KUBECTL_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile} | |
sed -i "s/ENV HELM_VERSION .*/ENV HELM_VERSION ${HELM_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile} | |
sed -i "s/ENV KUSTOMIZE_VERSION .*/ENV KUSTOMIZE_VERSION ${KUSTOMIZE_VERSION}/" ${DOCKERFILE_DIR}/${dockerfile} | |
sed -i "s/ENV KUBECTL_VERSION .*/ENV KUBECTL_VERSION ${KUBECTL_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps | |
sed -i "s/ENV HELM_VERSION .*/ENV HELM_VERSION ${HELM_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps | |
sed -i "s/ENV KUSTOMIZE_VERSION .*/ENV KUSTOMIZE_VERSION ${KUSTOMIZE_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps | |
sed -i "s/ENV KUBECTL_VERSION .*/ENV KUBECTL_VERSION ${KUBECTL_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps.lts | |
sed -i "s/ENV HELM_VERSION .*/ENV HELM_VERSION ${HELM_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps.lts | |
sed -i "s/ENV KUSTOMIZE_VERSION .*/ENV KUSTOMIZE_VERSION ${KUSTOMIZE_VERSION}/" ${DOCKERFILE_DIR}/Dockerfile.deps.lts |
Description
Removing final references to slim in /hack