Extend selectors to include provider name and class #4583
Merged
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
jhrozek
commented
Sep 30, 2024
| //go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE | ||
|
|
||
| // RepoSelectorConverter is an interface for converting a repository to a repository selector | ||
| type RepoSelectorConverter interface { |
There was a problem hiding this comment.
These interfaces are no longer needed because we're operating on properties only and there is nothing provider-specific at this point.
jhrozek
commented
Sep 30, 2024
| ) | ||
|
|
||
| func repoToSelectorEntity(t *testing.T, name, class string, repoEntWithProps *models.EntityWithProperties) *internalpb.SelectorEntity { |
There was a problem hiding this comment.
this is removal of the provider interface that is no longer needed.
jhrozek
commented
Sep 30, 2024
| prWithProperties(map[string]any{ | ||
| "github": map[string]any{"is_draft": true}, | ||
| }), | ||
| ), | ||
| selected: false, | ||
| }, | ||
| { | ||
| name: "Use an artifact property that is defined and true result", |
There was a problem hiding this comment.
these tests are not related to this PR, but the linter was yelling at me that the artifacts options are not used, so I added cases using them
jhrozek
commented
Sep 30, 2024
| @@ -45,6 +48,8 @@ Let's break down the example above: | |||
| - The first selector filters repositories to include only those that are not forks and whose name starts with stacklok. In other words, those that are part of the stacklok organization. | |||
| - The second selector filters artifacts to include only those provided by `github-app-stacklok`. | |||
| - The third selector filters repositories to include only those with a GPL license and the fourth selector filters repositories to include only those written in Go. These two selectors use the properties map which is provider-specific. | |||
| - The fourth selector filters repositories to include only that use Go as the primary language. | |||
| - The fifth selector filters repositories to include only those provided by the GitHub provider. | |||
There was a problem hiding this comment.
it's a bit embarassing but we already had documentation on the provider fields. So the only docs I added are examples.
jhrozek
force-pushed
the
selector_provider
branch
from
1204cdd to
22cc2a7
Compare
|
oops forgot to adjust that unit test apparently. Fixing! |
jhrozek
force-pushed
the
selector_provider
branch
from
22cc2a7 to
d52f9a6
Compare
JAORMX
previously approved these changes
Oct 1, 2024
…equestSelectorConverter provider-specific interfaces Before properties we were filling the SelectorEntity proto message from bespoke combinations constructed from the entity protobufs like pb.Repository. Since we now handle entities in a generic manner we don't need this interface at all. Related: mindersec#4564
We want to use it in code that we are moving to be provider-agnostic. Plus, it makes sense, artifacts from any provider will always have a 'type'. Related: mindersec#4564
… period of time In the unlikely case anyone still relies on the old property, let's fall back to it until it disappears. Related: mindersec#4564
It was missing! Related: mindersec#4564
In order to be able to evaluate selectors including the provider, we need to fill in the provider information. Related: mindersec#4564
We need to test our code Related: mindersec#4564
Embarassingly enough we already had some documentation on the provider selectors before we implemented them, so let's just add an example. Related: mindersec#4564
jhrozek
force-pushed
the
selector_provider
branch
from
d52f9a6 to
5319c56
Compare
|
I rebased atop the current main. There are no other changes. |
JAORMX
approved these changes
Oct 1, 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.
Summary
We had included provider name and class bundled into an internal proto message already initially when implementing the first pass on selectors.
But we never filled in the structures, so the provider selectors didn't really work.
I took the opportunity to remove one interface that we don't need anymore that was converting the typed protobuf messages to selector entitites.
This is not needed anymore since all entities can be expressed as properties.
This is a large PR, but most of the diff is deletions of obsolete code and adding tests. I will point out in the PR what is what.
Fixes #4564
Change Type
Testing
unit tests + manual testing
Review Checklist: