Skip to content

chore: cleaning up final references to slim from skaffold#9616

Merged
plumpy merged 1 commit intoGoogleContainerTools:mainfrom
Darien-Lin:remove-lingering-slim-references
Dec 13, 2024
Merged

chore: cleaning up final references to slim from skaffold#9616
plumpy merged 1 commit intoGoogleContainerTools:mainfrom
Darien-Lin:remove-lingering-slim-references

Conversation

@Darien-Lin
Copy link
Copy Markdown
Contributor

Description
Removing final references to slim in /hack

@Darien-Lin Darien-Lin requested a review from a team as a code owner December 13, 2024 17:56
@Darien-Lin Darien-Lin requested a review from mattsanta December 13, 2024 17:56
Copy link
Copy Markdown

@ghost ghost left a 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]

@Darien-Lin Darien-Lin requested a review from plumpy December 13, 2024 17:57
Copy link
Copy Markdown

@ghost ghost left a 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.

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

low

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.

Suggested change
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)

Comment on lines 118 to 120
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}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

low

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.

Suggested change
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

@plumpy plumpy merged commit f185889 into GoogleContainerTools:main Dec 13, 2024
@Darien-Lin Darien-Lin deleted the remove-lingering-slim-references branch February 24, 2026 19:51
@menahyouyeah menahyouyeah mentioned this pull request Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants