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

Add a shared actions context structure #5182

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Dec 11, 2024

Summary

This shared space will allow actions to gather structures or results
that will allow it to aggregate and flush. In more solid terms, we'd be
able to aggregate PR comments, iterate them, and issue just one big
comment.

The intention is for drivers (e.g. the pull request comment alert type) to fetch this from the context and push shared statements (e.g. comments), plus an aggregation handling function that meets the relevant signature. The aggregation function will be called in the end, thus allowing it to gather all comments and publish just one result.

Fixes #5119

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

This shared space will allow actions to gather structures or results
that will allow it to aggregate and flush. In more solid terms, we'd be
able to aggregate PR comments, iterate them, and issue just one big
comment.

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
@JAORMX JAORMX requested a review from a team as a code owner December 11, 2024 13:11
@JAORMX JAORMX marked this pull request as draft December 11, 2024 13:11
@JAORMX JAORMX self-assigned this Dec 11, 2024
@coveralls
Copy link

Coverage Status

coverage: 55.323% (-0.03%) from 55.352%
when pulling b403b5c on JAORMX:shared-actions-context
into f2d6feb on mindersec:main.

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

How do we see this aggregation working? To quote from #5119 (comment)

If license scanning takes 100ms and static analysis takes 5 minutes, do we add the license scan report right away and then update the comment later when static analysis comes in?

I'm having a little trouble visualizing how this works -- I think this assumes that all the rule eval and actions for a given event happen on the same Minder server? And we'll end up with something on the event-handling context that allows "enqueue this to be done in the future"?

Thinking about audit logs and splitting action execution from action computation, I'd love to have a data structure for collecting these actions, rather than a list of functions. Ideallly, this would look like:

{notification / reminder edge} --> [visit $foo resource]
[visit $foo resource] -> (per-rule) {rule-eval} -> (per-rule) [eval results] & [action plan?]
[action plan] -> {action-processor} -> [action result]
[eval results] -> {audit-log}
[action result] -> {audit-log}

Where [message]s are in brackets, and {computation} is in braces.

@@ -138,21 +138,23 @@ func (e *executor) EvalEntityEvent(ctx context.Context, inf *entities.EntityInfo
return fmt.Errorf("error while retrieving profiles and rule instances: %w", err)
}

sacctx, sac := actions.WithSharedActionsContext(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Why not still use ctx for sacctx?

@JAORMX
Copy link
Contributor Author

JAORMX commented Dec 12, 2024

How do we see this aggregation working? To quote from #5119 (comment)

If license scanning takes 100ms and static analysis takes 5 minutes, do we add the license scan report right away and then update the comment later when static analysis comes in?

I'm having a little trouble visualizing how this works -- I think this assumes that all the rule eval and actions for a given event happen on the same Minder server?

Right, it assumes an set of profiles will be evaluated for one event in a single Minder server. As is the case today.

And we'll end up with something on the event-handling context that allows "enqueue this to be done in the future"?

I was thinking of kicking it off immediately for now. We probably need to add such mechanism later when the use-case comes.

Thinking about audit logs and splitting action execution from action computation, I'd love to have a data structure for collecting these actions, rather than a list of functions. Ideallly, this would look like:

{notification / reminder edge} --> [visit $foo resource]
[visit $foo resource] -> (per-rule) {rule-eval} -> (per-rule) [eval results] & [action plan?]
[action plan] -> {action-processor} -> [action result]
[eval results] -> {audit-log}
[action result] -> {audit-log}

Where [message]s are in brackets, and {computation} is in braces.

The eval params gets us partly there, the main missing piece is the action results. Let me give that part some thought.

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 this pull request may close these issues.

Combine all Minder PR comments into a single comment
3 participants