-
Notifications
You must be signed in to change notification settings - Fork 499
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: move merge logic to an hcl package and add tests #2831
base: master
Are you sure you want to change the base?
Conversation
0dbd6cb
to
94b9987
Compare
Signed-off-by: CrazyMax <[email protected]>
94b9987
to
ff00ac1
Compare
@@ -1,9 +1,7 @@ | |||
// Copyright (c) HashiCorp, Inc. | |||
// SPDX-License-Identifier: MPL-2.0 | |||
|
|||
// Forked from https://github.com/hashicorp/hcl/blob/4679383728fe331fc8a6b46036a27b8f818d9bc0/merged.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be kept?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we should as this now has the same hierarchy as upstream repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe we could have a README in this package
@@ -16,6 +16,7 @@ import ( | |||
|
|||
composecli "github.com/compose-spec/compose-go/v2/cli" | |||
"github.com/docker/buildx/bake/hclparser" | |||
hclalt "github.com/docker/buildx/bake/hclparser/hcl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this package is only to be used internally, and not directly used by others, it's worth considering moving it to an internal/
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buildx is just a client and I don't really like the internal
pattern but I don't mind if we want to move that there.
follow-up #2758 (comment)