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.

Sign up for GitHub

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

Jump to bottom

bake: initial set of composable bake attributes #2758

Merged
merged 1 commit into from
Nov 21, 2024
Merged

bake: initial set of composable bake attributes #2758

merged 1 commit into from
Nov 21, 2024

Conversation

jsternberg
Copy link
Collaborator

@jsternberg jsternberg commented Oct 28, 2024 •
edited
Loading

This allows using either the csv syntax or object syntax to specify
certain attributes.

This applies to the following fields:

  • output
  • cache-from
  • cache-to
  • secret
  • ssh

There are still some remaining fields to translate. Specifically
ulimits, annotations, and attest.

Related to #438.

@jsternberg
Copy link
Collaborator Author

Very incomplete first draft. This requires some changes to the gohcl package in the hcl package for it to work. The biggest thing is overriding the ImpliedType method and copying the full implementation from go-cty so that we can customize the behavior for certain types. In this case, I create a FromCtyValue interface and that works for our purposes.

The only changes to gohcl is creating a DecodeOptions which allows us to inject our own version of ImpliedType into the decoder. I also made it so we can change convert, but modifying the convert implementation was a lot more involved than modifying ImpliedType and using the existing capsule type.

tonistiigi
tonistiigi reviewed Oct 28, 2024
View reviewed changes
bake/types.go Outdated Show resolved Hide resolved
@jsternberg jsternberg force-pushed the bake-composable-attributes branch 2 times, most recently from 2a36f74 to 88ff1f4 Compare October 30, 2024 19:14
@jsternberg jsternberg mentioned this pull request Oct 30, 2024
Open
@jsternberg jsternberg force-pushed the bake-composable-attributes branch 4 times, most recently from ceecb9d to af7ebbf Compare November 4, 2024 18:21
@jsternberg jsternberg force-pushed the bake-composable-attributes branch 2 times, most recently from 95dc438 to 3f474cc Compare November 5, 2024 17:53
@jsternberg
Copy link
Collaborator Author

I've modified this to include a small fork of the gohcl package temporarily while we resolve any legal stuff getting the CLA signed for upstream.

@crazy-max
Copy link
Member

crazy-max commented Nov 6, 2024 •
edited
Loading

I've modified this to include a small fork of the gohcl package temporarily while we resolve any legal stuff getting the CLA signed for upstream.

https://github.com/docker/buildx/blob/master/bake/hclparser/merged.go is also a fork from upstream. Maybe we could move it to an hcl package under hclparser one and then move LICENSE and gohcl in hcl?

./bake/
├── ...
├── hclparser
│   ├── ...
│   ├── hcl
│   │   ├── LICENSE
│   │   ├── gohcl
│   │   └── merged.go
│   ├── ...
└── ...

@jsternberg jsternberg force-pushed the bake-composable-attributes branch 2 times, most recently from 6b42716 to 5f76a38 Compare November 8, 2024 16:51
@jsternberg jsternberg changed the title bake: prototype for composable bake attributes bake: initial set of composable bake attributes Nov 8, 2024
@jsternberg
Copy link
Collaborator Author

This PR is becoming fairly large. There are a few more attributes that have to be migrated to work, but it might be good to review and merge what we have to land this feature. I'm changing this to ready to review while I work on the remaining attributes.

@jsternberg jsternberg marked this pull request as ready for review November 8, 2024 17:07
@jsternberg jsternberg force-pushed the bake-composable-attributes branch 2 times, most recently from 1ed12f2 to dcf5387 Compare November 8, 2024 21:15
@thompson-shaun thompson-shaun added this to the v0.19.0 milestone Nov 13, 2024
@crazy-max
Copy link
Member

Overall LGTM but still testing other cases like the matrix one so bare with me but would be nice to have more test cases.

@thompson-shaun thompson-shaun linked an issue Nov 14, 2024 that may be closed by this pull request
bake: more structured fields instead csv #438
Closed
@thompson-shaun thompson-shaun modified the milestones: v0.19.0, v0.20.0 Nov 14, 2024
@jsternberg
Copy link
Collaborator Author

Putting this back to draft for the time being since the variable references don't work.

@jsternberg jsternberg marked this pull request as draft November 15, 2024 21:45
@crazy-max crazy-max mentioned this pull request Nov 18, 2024
Merged
@thompson-shaun thompson-shaun modified the milestones: v0.20.0, v0.19.0 Nov 19, 2024
@jsternberg
Copy link
Collaborator Author

Ready for review again. Variable resolution works. The key part is HCL doesn't support the capsule types when resolving variables so there's now code that converts the capsule types to native types when resolving variables.

@jsternberg jsternberg marked this pull request as ready for review November 20, 2024 19:25
@jsternberg jsternberg force-pushed the bake-composable-attributes branch 2 times, most recently from ba03523 to 4070cb4 Compare November 20, 2024 19:56
tonistiigi
tonistiigi reviewed Nov 20, 2024
View reviewed changes
bake/hcl_test.go
require.Equal(t, []string{"default", "key=path/to/key"}, stringify(c.Targets[0].SSH))
}

func TestHCLAttrsCapsuleTypeVars(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Add also cases for

  • accessing another target, not just properties of the same target
  • using global variables in the object values
  • mixing csv and objects (whatever the behavior is just to lock it down)

Copy link
Member

Choose a reason for hiding this comment

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

  • array assignment cache-to = target.app.cache-from

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@jsternberg
bake: initial set of composable bake attributes
3ccbb88 
This allows using either the csv syntax or object syntax to specify
certain attributes.

This applies to the following fields:
- output
- cache-from
- cache-to
- secret
- ssh

There are still some remaining fields to translate. Specifically
ulimits, annotations, and attest.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
@tonistiigi tonistiigi merged commit 1a03911 into docker:master Nov 21, 2024
122 checks passed
@jsternberg jsternberg deleted the bake-composable-attributes branch November 21, 2024 21:21
@crazy-max crazy-max mentioned this pull request Nov 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@crazy-max crazy-max Awaiting requested review from crazy-max

Assignees

@tonistiigi tonistiigi

@crazy-max crazy-max

Labels
area/bake/compose area/bake area/controller area/dependencies area/tests needs/docs-follow-up
Projects
None yet
Milestone
v0.19.0
Development

Successfully merging this pull request may close these issues.

bake: more structured fields instead csv
4 participants
@jsternberg @crazy-max @tonistiigi @thompson-shaun