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

bake: various fixes for composable attributes #2814

Merged

Conversation

jsternberg
Copy link
Collaborator

@jsternberg jsternberg commented Nov 22, 2024

This changes how the composable attributes are implemented and provides
various fixes to the first iteration.

Cache-from and cache-to now no longer print sensitive values that are
automatically added. These automatically added attributes are added when
the protobuf is created rather than at the time of parsing so they will
no longer be printed. If they are part of the original configuration
file, they will still be printed.

Empty strings will now be skipped. This was the original behavior and
composable attributes removed this functionality accidentally. This
functionality is now restored.

This also expands the available syntax that works with each of the
composable attributes. It is now possible to interleave the csv syntax
with the object syntax without any problems. The canonical form is still
the object syntax and variables are resolved according to that syntax.

Fixes #2823.
Fixes #2822.
Fixes #2858

Replaces #2832.
Replaces #2833.

@jsternberg jsternberg force-pushed the bake-composable-attributes-phase2 branch from 72a566c to 73f055d Compare November 25, 2024 18:29
@jsternberg jsternberg force-pushed the bake-composable-attributes-phase2 branch from 73f055d to 70c1cad Compare December 6, 2024 21:05
@jsternberg jsternberg force-pushed the bake-composable-attributes-phase2 branch 2 times, most recently from 8eafb54 to 753705d Compare December 9, 2024 16:04
@jsternberg jsternberg force-pushed the bake-composable-attributes-phase2 branch 2 times, most recently from d03e42f to 4e32acc Compare December 9, 2024 21:57
@jsternberg jsternberg changed the title bake: implement composable attribute for attest bake: various fixes for composable attributes Dec 9, 2024
@jsternberg jsternberg requested a review from tonistiigi December 9, 2024 21:59
@jsternberg jsternberg force-pushed the bake-composable-attributes-phase2 branch from 4e32acc to 91f0596 Compare December 9, 2024 21:59
@jsternberg jsternberg marked this pull request as ready for review December 9, 2024 22:00
@jsternberg jsternberg force-pushed the bake-composable-attributes-phase2 branch 2 times, most recently from e86f8b3 to e466b5b Compare December 9, 2024 22:08
@crazy-max crazy-max force-pushed the bake-composable-attributes-phase2 branch from e466b5b to d0212a9 Compare December 11, 2024 11:29
@crazy-max
Copy link
Member

rebased to fix xx issue

@crazy-max crazy-max added this to the v0.20.0 milestone Dec 11, 2024
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Tested on my side on various projects and looks great!

tests/bake.go Outdated Show resolved Hide resolved
Equal(other E) bool
}

func removeDupes[E comparable[E]](s []E) []E {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be duplicated. Additionally (not for this PR) there is also removeDupesStr but it uses map. I think this could also avoid N^2 complexity but not sure if worth it as the inputs are likely small.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The purpose of this was it could utilize the Equal method that was added so it wasn't so dependent on strings. Due to the small number of inputs, the N^2 complexity isn't likely to be an issue. This might actually be faster than removeDupesStr just because maps have some issues when used for deduplicating small inputs due to overhead from memory allocation and hash key lookups.

This was duplicated and I've removed the duplicate from bake/bake.go. When I originally did this, it was in bake/bake.go and then I started moving stuff. It was easier to just copy it at the time. Since I missed the change to call Normalize() for t.SSH as pointed out in the other comment, the linter didn't catch that the original call was no longer needed since all calls had been moved to this file. I've changed that to use Normalize() and removed the duplicate.

bake/bake.go Outdated Show resolved Hide resolved
bake/bake.go Show resolved Hide resolved
This changes how the composable attributes are implemented and provides
various fixes to the first iteration.

Cache-from and cache-to now no longer print sensitive values that are
automatically added. These automatically added attributes are added when
the protobuf is created rather than at the time of parsing so they will
no longer be printed. If they are part of the original configuration
file, they will still be printed.

Empty strings will now be skipped. This was the original behavior and
composable attributes removed this functionality accidentally. This
functionality is now restored.

This also expands the available syntax that works with each of the
composable attributes. It is now possible to interleave the csv syntax
with the object syntax without any problems. The canonical form is still
the object syntax and variables are resolved according to that syntax.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
@jsternberg jsternberg force-pushed the bake-composable-attributes-phase2 branch from 15d291c to 5dd4ae0 Compare December 18, 2024 16:26
@tonistiigi tonistiigi merged commit 3771fe2 into docker:master Dec 18, 2024
122 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants