Refactor engine code to use specific provider traits in more places #3262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JAORMX
requested changes
May 7, 2024
| @@ -43,7 +43,11 @@ func NewRuleAlert(rt *pb.RuleType, pbuild *providers.ProviderBuilder) (engif.Act | |||
| if alertCfg.GetSecurityAdvisory() == nil { | |||
| return nil, fmt.Errorf("alert engine missing security-advisory configuration") | |||
| } | |||
| return security_advisory.NewSecurityAdvisoryAlert(ActionType, rt.GetSeverity(), alertCfg.GetSecurityAdvisory(), pbuild) | |||
| client, err := pbuild.GetGitHub() | |||
There was a problem hiding this comment.
Can we first check if the provider implements github and skip if it doesn't?
There was a problem hiding this comment.
I'm not sure I agree with this suggestion:
- The code copies the existing behaviour: https://github.com/stacklok/minder/blob/main/internal/engine/actions/alert/security_advisory/security_advisory.go#L161
- If other aspects of the configuration are incorrect, we also return an error.
There was a problem hiding this comment.
Note in my next PR, I will change the behaviour to print an explicit error if the provider is not Github
There was a problem hiding this comment.
There is an explicit "error" that gets the engine to mark something as skipped. Anyway, it's fine to do it in a separate PR.
JAORMX
reviewed
May 7, 2024
Relates to #2845 When evaluating a policy, we instantiate a ProviderBuilder and then pass it around to various parts of the code. Once the specific trait is known, we get the specific trait we need from the ProviderBuilder. While replacing the ProviderBuilder, I notice that there are many parts of the code where we continue to pass around the ProviderBuilder even though we know the specific provider trait which will be needed. I changed various code to explicitly require the provider trait it needs instead of taking the ProviderBuilder and getting an instance of the desired type. As a result of this change, test setup is simplified, and this will make my PR to replace the ProviderBuilder smaller.
dmjb
force-pushed
the
engine-provider-traits
branch
from
6847474
to
a101fb2
Compare
JAORMX
approved these changes
May 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relates to #2845
When evaluating a policy, we instantiate a ProviderBuilder and then pass it around to various parts of the code. Once the specific trait is known, we get the specific trait we need from the ProviderBuilder.
While replacing the ProviderBuilder, I notice that there are many parts of the code where we continue to pass around the ProviderBuilder even though we know the specific provider trait which will be needed. I changed various code to explicitly require the provider trait it needs instead of taking the ProviderBuilder and getting an instance of the desired type. As a result of this change, test setup is simplified, and this will make my PR to replace the ProviderBuilder smaller.
Summary
Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.
Fixes #(related issue)
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: