Skip to content

Fix lost diags across different mutators#2057

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

Merged
denik merged 1 commit into from
Dec 31, 2024
Merged

Fix lost diags across different mutators #2057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bundle/artifacts/expand_globs.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -97,7 +97,7 @@ func (m *expandGlobs) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnost
return dyn.SetByPath(v, base, dyn.V(output))
})
if err != nil {
return diag.FromErr(err)
diags = diags.Extend(diag.FromErr(err))
}

return diags
Expand Down
1 change: 1 addition & 0 deletions bundle/config/mutator/resolve_resource_references.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -40,6 +40,7 @@ func (m *resolveResourceReferences) Apply(ctx context.Context, b *bundle.Bundle)
})
}

// Note, diags are lost from all goroutines except the first one to return diag

@andrewnesterandrewnesterDec 31, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shall we fix it instead?

@denikdenikDec 31, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I don't know of an easy way - one would need to synchronize access to some common data structure - and I'd rather not invest a lot of time there - as this won't be necessary if move away from returning diags.

@andrewnesterandrewnesterDec 31, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can't it just be as simple as

mu.Lock()
diags = diags.Append(...)
mu.Unlock()

inside the goroutine?

@denikdenikDec 31, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe - we can discuss offline. I'd consider this out of scope for this PR though (it does not need to be perfect, just incrementally better).

@denikdenikDec 31, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

There are more cases of using errgroup and diags together. So if we want to solve it properly, we should probably have some sort of abstraction that makes the usage foolproof.

However, like I said above, I'd rather not invest there because at this point it seems that the best abstraction is to call a function as soon as you have a diagnostic message, not accumulate them.

return diag.FromErr(errs.Wait())
}

Expand Down
3 changes: 2 additions & 1 deletion bundle/config/root.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,8 @@ func LoadFromBytes(path string, raw []byte) (*Root, diag.Diagnostics) {
// Convert normalized configuration tree to typed configuration.
err = r.updateWithDynamicValue(v)
if err != nil {
return nil, diag.Errorf("failed to load %s: %v", path, err)
diags = diags.Extend(diag.Errorf("failed to load %s: %v", path, err))
return nil, diags
}
return &r, diags
}
Expand Down
3 changes: 2 additions & 1 deletion bundle/config/validate/folder_permissions.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -36,7 +36,8 @@ func (f *folderPermissions) Apply(ctx context.Context, b bundle.ReadOnlyBundle)
}

if err := g.Wait(); err != nil {
return diag.FromErr(err)
// Note, only diag from first coroutine is captured, others are lost
diags = diags.Extend(diag.FromErr(err))
}

for _, r := range results {
Expand Down