refactor: populate provider field in entity context using provider ID fallback by sachin9058 · Pull Request #6281 · mindersec/minder

sachin9058 · 2026-04-05T13:42:01Z

Summary

Populate the provider field in the entity context using the provider ID as a fallback.

Previously, the provider field was not set in the entity context. Since provider name resolution is not available at this layer, the provider ID is used as a fallback to improve observability and debugging.

This change keeps the implementation simple and avoids introducing additional dependencies into the handler layer.

Fixes #NONE

Testing

Ran make lint-fix (no issues)
Ran go test ./... -count=1 (all tests passing)

coveralls · 2026-04-05T13:52:26Z

Coverage is 60.446% — sachin9058:refactor-provider-context-clean into mindersec:main. No base build found for mindersec:main.

evankanderson

The ProviderID is a UUID, and won't really be very useful in the context, while the provider name tends to encode some actual information (like "githubapp-$install-info").

I'm not sure that this TODO is fixed by adding a UUID to the entity context.

sachin9058 · 2026-04-06T09:56:12Z

Addressed the feedback and fixed the provider lookup to use the store instead of the UUID fallback. Also updated tests and resolved lint issues.

All checks are passing now — please take another look.

evankanderson

This seems to have a lot of spurious changes, like #6278 . Can you reduce the size of the diff, to make it clearer what the intended meaningful changes are?

sachin9058 · 2026-04-06T21:41:22Z

@evankanderson Thanks for the feedback.

I’ve removed the unrelated changes and simplified the PR to keep it focused and minimal. The diff should now only reflect the intended behavior and be easier to review.

Please take another look.

evankanderson

This has a lot of spurious diffs; enough that it's hard for me to see what the intent of the PR is. Can you revert the changes to comments and formatting in unrelated areas? If you're using AI, you may need to include a "produce minimal changes" in the prompt.

evankanderson

I'm marking this PR as "request changes" (this still has too many spurious comment, etc changes) to help track which outstanding PRs need maintainer action vs contributor action.

sachin9058 · 2026-04-28T21:08:01Z

@evankanderson I’ve cleaned up the branch history and reduced the PR to the intended provider-context change only. The branch now contains a single focused commit that restores the provider-name lookup through the store, keeps the handler comments intact, and updates the corresponding test and service wiring. I also verified the relevant tests pass locally. Please take another look.

sachin9058 · 2026-04-28T21:45:09Z

@evankanderson I also fixed the make test-cover-silent failure in test.mk that was crashing in CI. The issue was the gotestfmt formatter panicking on the coverage output, so I switched the target to run go test under bash -o pipefail and stream output through tee directly to test-results.json. The coverage run now completes cleanly locally.

evankanderson · 2026-04-29T22:30:11Z

 .PHONY: test-cover-silent
 test-cover-silent: clean init-examples  ## Run test coverage in a silent mode (errors only output)
-	go test -json -race -v -coverpkg=${COVERAGE_PACKAGES} -coverprofile=coverage.out.tmp ./... 2>&1 | tee test-results.json | gotestfmt -hide "all"
+	bash -o pipefail -c 'go test -json -race -v -coverpkg=${COVERAGE_PACKAGES} -coverprofile=coverage.out.tmp ./... 2>&1 | tee test-results.json >/dev/null'


This removes all the direct output from running make-test-silent. I can buy that, but it seems like test-silent should do the same thing or be removed then.

What I'd ideally like would be:

Get test coverage and junit (xml) reports

Get a summary of failed tests to the terminal.

1 supports coveralls and a (to-be-written) cross-run summary tool

2 supports interactive use and quick clicking through the workflow results.

evankanderson · 2026-04-29T22:32:56Z

 }

-// Wait waits for all the entity executions to finish.
+// Wait blocks until all entity executions are complete.


You're still rewriting a lot of comments in this diff (this function didn't seem to change).

evankanderson · 2026-04-29T22:34:04Z

 	e.cancels = append(e.cancels, &shutdownCancel)
 	e.lock.Unlock()

-	// Let's not share memory with the caller.  Note that this does not copy Context


I suspect that this comment was added alongside the following line after an extended debugging session. Let's not remove it.

evankanderson · 2026-04-29T22:34:35Z


-		ctx, cancel := context.WithTimeout(msgCtx, DefaultExecutionTimeout)
+		timeout := e.executionTimeout
+		if timeout <= 0 {


How can this happen?

evankanderson · 2026-04-29T22:36:33Z

 		}
 		ts.Record(logMsg).Send()

+		// record telemetry regardless of error. We explicitly record telemetry


Why has this comment moved after the action it describes?

evankanderson

I'm okay with the described change (adding a database lookup in order to include the provider in the message context), but there's a lot of other changes going on in this PR as well that need to either be reverted or explained.

evankanderson · 2026-05-07T13:45:55Z

-	@echo "Running docker compose down $(services)..."
-	@$(COMPOSE) down $(services)
+	@echo "Running docker compose down..."
+	@$(COMPOSE) down


These changes seem unrelated. Revert?

evankanderson · 2026-05-07T13:49:10Z

 .PHONY: test-silent
 test-silent: clean init-examples ## run tests in a silent mode (errors only output)
-	go test -json -race -v ./... | gotestfmt -hide "all"
+	bash -o pipefail -c 'go test -json -race -v ./... 2>&1 | tee test-results.json >/dev/null'


Why use the construct 2>&1 | tee $FILE >/dev/null rather than &> $FILE (at which point you might not need -o pipefail as well).

Also, I don't see the CI changes documented in the PR description, so I was surprised to find them here.

evankanderson · 2026-05-07T13:51:57Z

 	if e.closed {
 		e.lock.Unlock()
 		shutdownCancel()
 		return nil
 	}


I don't see this mentioned in the PR description -- what's this doing?

evankanderson · 2026-05-07T13:54:21Z

 	evt interfaces.Publisher,
 	handlerMiddleware []message.HandlerMiddleware,
 	executor Executor,
+	executionTimeout time.Duration,


This seems to also be merging in a version of #6278 . Intentional?

sachin9058 requested a review from a team as a code owner April 5, 2026 13:42

evankanderson reviewed Apr 6, 2026

View reviewed changes

sachin9058 force-pushed the refactor-provider-context-clean branch from 6405039 to 9e18d08 Compare April 7, 2026 14:52

evankanderson reviewed Apr 8, 2026

View reviewed changes

Comment thread internal/engine/handler.go

evankanderson requested changes Apr 28, 2026

View reviewed changes

Resolve provider context lookup

52feb7d

sachin9058 force-pushed the refactor-provider-context-clean branch from 9e18d08 to 52feb7d Compare April 28, 2026 21:06

sachin9058 requested a review from evankanderson April 28, 2026 21:09

sachin9058 added 3 commits April 29, 2026 02:56

Fix coverage test target

e5d9606

ci: retrigger checks

c6b1060

Add compatible executor handler constructor

cb6dd0a

evankanderson requested changes Apr 29, 2026

View reviewed changes

sachin9058 added 3 commits April 30, 2026 13:42

Restore handler comments and wiring

d36f94e

Fix silent test targets

c5329a1

Fix docker teardown targets

6fb8dad

evankanderson reviewed May 7, 2026

View reviewed changes

Conversation

sachin9058 commented Apr 5, 2026

Summary

Testing

Uh oh!

coveralls commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

sachin9058 commented Apr 6, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

sachin9058 commented Apr 6, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

sachin9058 commented Apr 28, 2026

Uh oh!

sachin9058 commented Apr 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Apr 5, 2026 •

edited

Loading

evankanderson May 7, 2026 •

edited

Loading