Support the release entity for the GitHub provider #4921
Conversation
JAORMX
force-pushed
the
github-releases
branch
2 times, most recently
from
843b3f0 to
6f98655
Compare
JAORMX
force-pushed
the
github-releases
branch
2 times, most recently
from
ec7b12f to
d0081f5
Compare
JAORMX
changed the title
There was a problem hiding this comment.
Does this create release entities, or just set us up for doing so in the future?
It looks (right now), like there's still more work before you can write a rule against these release objects -- are they created automatically (like PRs) after registering a repo?
I'm assuming (like PRs), that we will only start looking at releases that are modified after Minder is installed?
| v1 "github.com/mindersec/minder/pkg/providers/v1" | ||
| ) | ||
|
|
||
| // Release Properties |
There was a problem hiding this comment.
I'm assuming this will grow over time as we find the other useful properties.
There was a problem hiding this comment.
that's right. This applies to all of our entity properties.
| } | ||
|
|
||
| func getReleaseNameFromParams(owner, repo, tag string) string { | ||
| if owner == "" { |
There was a problem hiding this comment.
This seems like a weird case -- does it actually happen, or could we put in a complaint?
There was a problem hiding this comment.
I'll be honest, I cargo-culted this. I'm not sure 😄
There was a problem hiding this comment.
Let's make this an error and/or log at warning/error -- I think it's valid input to the GitHub API, but we shouldn't be getting output in this non-normalized form.
It creates the release entities.
Like PRs, these are created after registering a repo, as they get created. You could already start writing policies for these; the main issue we have right now is that we don't have an API to get general entities like these. |
| } | ||
|
|
||
| func getReleaseNameFromParams(owner, repo, tag string) string { | ||
| if owner == "" { |
There was a problem hiding this comment.
Let's make this an error and/or log at warning/error -- I think it's valid input to the GitHub API, but we shouldn't be getting output in this non-normalized form.
There was a problem hiding this comment.
A few more comments.
It looks like GitHub uses the tag name as the unique identifier for releases, and you can have duplicate "name" (titles, really) for releases, but that my not be universal -- it feels like getReleaseNameFromParams will probably need to vary based on provider.
|
|
||
| return map[string]any{ | ||
| properties.PropertyUpstreamID: properties.NumericalValueToUpstreamID(release.GetID()), | ||
| properties.PropertyName: getReleaseNameFromParams(owner, repo, release.GetTagName()), |
There was a problem hiding this comment.
It looks like GitHub's name here is non-unique, but may still be worth exposing for e.g. filtering with CEL. (Apply / omit evalution only to releases with certain names)
| // ReleasePropertyTag represents the github release tag name. | ||
| ReleasePropertyTag = "github/tag" | ||
| // ReleasePropertyBranch represents the github release branch | ||
| ReleasePropertyBranch = "github/branch" |
There was a problem hiding this comment.
It was suggested we should have a common-top-level property for these, and for the commit SHA.
JAORMX
force-pushed
the
github-releases
branch
from
d0081f5 to
99ce81d
Compare
There was a problem hiding this comment.
One nit on https://github.com/mindersec/minder/pull/4921/files#r1871541959 that I'd like to see us track (because having two different name formats makes everything else harder).
|
If we merge this, we should make sure to follow up with a PR to restore the -0.4% code coverage drop from adding this new code. |
Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
JAORMX
force-pushed
the
github-releases
branch
from
d0740ad to
d1078f4
Compare
Summary
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: