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

fix: Add Dockerfile for digest calculation #9666

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AndreasBergmeier6176
Copy link
Contributor

Fixes: #8226

Description
inputDigest should now take Dockerfiles into account for digest calculation.

Copy link

@code-review-assist code-review-assist bot 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 provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed:

This pull request, authored by AndreasBergmeier6176, aims to improve the digest calculation for images by incorporating Dockerfiles. The intent, as stated in the description, is to fix issue #8226.

Summary of Changes:

The core change is in pkg/skaffold/tag/input_digest.go. Specifically, lines 57-70 were modified to include Dockerfile paths in the digest calculation. The changes ensure that the following are now included:

  • Paths from image.DockerArtifact.DockerfilePath
  • Paths from image.KanikoArtifact.DockerfilePath
  • Paths from image.CustomArtifact.Dependencies.Dockerfile.Path (if available)

These paths are appended to the srcFiles slice, which is then sorted before being used in the digest calculation. This ensures that the order of files doesn't affect the final digest.

In short: The PR enhances the accuracy of image digest calculation by considering the content of associated Dockerfiles.


And now, a little haiku to lighten the mood:

Dockerfile's might,
Digest now more accurate,
Builds run smoothly on.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This change addresses issue #8226 by including Dockerfiles in the digest calculation. This is a good improvement. However, the implementation could be made more concise and robust. Also, consider adding tests to verify the changes.

Previously changes in Dockerfile did not lead to a new artifact being
built.
@mattsanta
Copy link
Contributor

Could you please update the PR title? - https://www.conventionalcommits.org/en/v1.0.0/

@AndreasBergmeier6176 AndreasBergmeier6176 changed the title Add Dockerfile for digest calculation fix: Add Dockerfile for digest calculation Jan 17, 2025
@mattsanta
Copy link
Contributor

Thanks for addressing the PR title. Once the linters are passing we can move forward with merging.

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.

inputDigest() build tagging policy doesn't take Dockerfile into account
2 participants