Include build args in cache hash generation

[priyawadhwa]

Fixes #2922

Description

Include build args in cache hash generation. This should fix the bug
described in #2922, where changing buildArgs resulted in the same
cache key, causing incorrect deployments.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes unit tests
• Mentions any output changes.
• Adds documentation as needed: user docs, YAML reference, CLI reference.
• Adds integration tests if needed.

Reviewer Notes

• The code flow looks good.
• Unit test added.
• User facing changes look good.

Release Notes

Describe any user facing changes here so maintainer can include it in the release notes, or delete this block.

Include build args in cache hash generation. This should fix the bug
described in #2922, where changing buildArgs resulted in the same
cache key, causing incorrect deployments.

[tejal29]

Add 2 more test cases. Rest LGTM

[Multiply]

Would it make sense to cover build args that use things like environment variables as well?

[codecov]

Codecov Report

Merging #2926 into master will increase coverage by 0.05%.
The diff coverage is 91.66%.

Impacted Files	Coverage Δ
pkg/skaffold/build/cache/hash.go	71.87% <91.66%> (+11.87%)	⬆️
pkg/skaffold/build/cluster/sources/sources.go	90.9% <0%> (-0.68%)	⬇️
pkg/skaffold/schema/v1beta10/config.go	100% <0%> (ø)	⬆️
pkg/skaffold/schema/v1alpha5/config.go	100% <0%> (ø)	⬆️
pkg/skaffold/schema/v1alpha1/config.go	100% <0%> (ø)	⬆️
pkg/skaffold/schema/v1beta1/config.go	100% <0%> (ø)	⬆️
pkg/skaffold/schema/v1beta7/config.go	100% <0%> (ø)	⬆️
pkg/skaffold/schema/v1beta6/config.go	100% <0%> (ø)	⬆️
pkg/skaffold/server/server.go	58.57% <0%> (ø)	⬆️
pkg/skaffold/schema/v1alpha3/config.go	100% <0%> (ø)	⬆️
... and 17 more

[priyawadhwa]

Hey @Multiply so build args actually don't support environment variable substitution right now in the skaffold config.

[Multiply]

@priyawadhwa I'm using them locally at the moment. (using buildkit)

[priyawadhwa]

@Multiply ah gotcha, I'll add that in

[tejal29]

Lets not do 2 things in one PR. Can we please move the environment substitution to another PR?

Thanks
Tejal

[priyawadhwa]

So we need to

1. do the environment substitution on the build args
2. pass in those build args to the hasher

I don't think it makes sense to split these up, because I can't write tests for the former without the code for the latter (since I'm testing that the hash at the end is correct. The environment substitution itself is a function in a different package which I'm reusing)

[tejal29]

So we need to

1. do the environment substitution on the build args
2. pass in those build args to the hasher

I don't think it makes sense to split these up, because I can't write tests for the former without the code for the latter (since I'm testing that the hash at the end is correct. The environment substitution itself is a function in a different package which I'm reusing)

Ah fair enough, you haven't written any extra logic for testing the environment solution. just 1 line change

buildArgs, err := docker.EvaluateBuildArgs(buildArgs)

[tejal29]

test nit.

[tejal29]

Failed due to TestGetSetFileValues/multiple_value out of order.