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: set attribute even if diagnosed as duplicated #1062

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Apr 11, 2022

Carry changes from #1025 related to an issue with hcl attributes not set if diagnosed as duplicated: #1025 (comment).

Needs to fork the merge logic from hcl repo. It might be better if hcl exposed its mergedbodies interface. Don't think we should fork the repo and vendor it here with this small change though. Maybe there is a better way?

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Did we try to upstream this?

Could we avoid copying this bunch of code by just providing our own JustAttributes() function that ranges over array and calls internal JustAttributes() for each item.

Add more context to this issue about the actual bake case it addresses and bake tests.

@crazy-max crazy-max marked this pull request as draft August 9, 2022 16:38
@jedevc
Copy link
Collaborator

jedevc commented Apr 14, 2023

@crazy-max do you have a test case here? (sorry, I know this is an older issue)

I think there might be an easier way to resolve this using some of the bake refactors recently, but I can't seem to reproduce the issue being discussed - the test from #1025 (comment) fails for different reasons.

@crazy-max
Copy link
Member Author

crazy-max commented Oct 20, 2023

do you have a test case here? (sorry, I know this is an older issue)

This is just for consistency with #1025 so we can merge global attributes like:

# c1.hcl
FOO = "abc"
target "app" {
  args = {
    v1 = "pre-${FOO}"
  }
}
# c2.hcl
FOO = "def"

Which currently gives v1 = pre-abc. I'm looking at upstreaming my change.

@crazy-max crazy-max force-pushed the bake-fix-attrs branch 2 times, most recently from 5f3e101 to 5846c05 Compare October 20, 2023 17:59
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
G601: Implicit memory aliasing in for loop. (gosec)

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
Copy link
Member Author

Did we try to upstream this?

Could we avoid copying this bunch of code by just providing our own JustAttributes() function that ranges over array and calls internal JustAttributes() for each item.

Add more context to this issue about the actual bake case it addresses and bake tests.

Opened an issue on HCL repo and got some feedback hashicorp/hcl#636 (comment). Creating our own implementation like Terraform does looks to be the right way.

@crazy-max crazy-max marked this pull request as ready for review October 20, 2023 20:12
@tonistiigi tonistiigi added this to the v0.12.0 milestone Oct 23, 2023
@tonistiigi tonistiigi merged commit deb9dbe into docker:master Oct 25, 2023
59 checks passed
@crazy-max crazy-max deleted the bake-fix-attrs branch October 25, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants