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

Incorrect paths being attributed to sections in generated output #23

Open
twelsh-aw opened this issue Dec 4, 2024 · 3 comments · May be fixed by #26
Open

Incorrect paths being attributed to sections in generated output #23

twelsh-aw opened this issue Dec 4, 2024 · 3 comments · May be fixed by #26
Assignees

Comments

@twelsh-aw
Copy link

First off, thanks for the tool. This is proving helpful to break things down and help us manage tailscale across networks and teams 🚀 .

Noticed that when multiple files are involved in a merge for an object or array property, that this ends up mis-attributing where the rules are actually sourced from.

Easiest repro is probably your testdata 😅:

// from `testdata/input-parent.hujson`

when in fact they come from somewhere else. It should say:

// from `testdata/departments/engineering/acls.hujson`

The underlying cause seems to be because the parent object is updated throughout and so this comment gets added on the second merge of things.

Same thing is also able to cause panics on merges of autoApprovers, but I'll file a separate issue for that and link back here.

@clstokes
Copy link
Collaborator

Thanks for reporting. I really need to redo the commenting logic and maybe the merge-into-parent logic (😅) so this one might take me a bit longer to fix.

@clstokes clstokes self-assigned this Jan 3, 2025
clstokes added a commit that referenced this issue Jan 3, 2025
fixes #23

also, updates existing testdata to be easier to follow original source file
@clstokes clstokes linked a pull request Jan 3, 2025 that will close this issue
@clstokes
Copy link
Collaborator

clstokes commented Jan 3, 2025

@twelsh-aw I have a work-in-progress fix in #26 if you'd like to give it a try - e.g. go run github.com/tailscale-dev/tailscale-acl-combiner@b1e2d62.

I want to rework the test data and tests a bit more before merging it - hopefully in the next few days.

@twelsh-aw
Copy link
Author

@clstokes beauty 🤩 . Will give it a spin later this week and let you know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants