Fix lost diags across different mutators #2057
Conversation
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
requested review from
pietern,
andrewnester and
shreyas-goenka
as code owners
denik
force-pushed
the
denik/do-not-lose-diags
branch
from
6681198 to
94dbac2
Compare
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| @@ -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 | |||
There was a problem hiding this comment.
Shall we fix it instead?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
can't it just be as simple as
mu.Lock()
diags = diags.Append(...)
mu.Unlock()
inside the goroutine?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Changes
Fix cases where accumulated diagnostics are lost instead of being propagated further. In some cases it's not possible, add a comment there.
Tests
Existing tests