-
Notifications
You must be signed in to change notification settings - Fork 0
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
Allow for adding new artifacts to an existing occurrence #4
Conversation
Id: extractOccurrenceIdFromName(occurrence.Name), | ||
Occurrence: occurrence, | ||
UpdateMask: &field_mask.FieldMask{ | ||
Paths: []string{"details.build.provenance.built_artifacts"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a grafeas-elasticsearch
issue to track this, but this field mask path is actually invalid, because details
is a oneof
field, it shouldn't appear in the mask path at all (same reason that it isn't serialized when converting a proto to text or json).
However, the utils library we use in grafeas-elasticsearch
ignores the mask if we don't specify the oneof
field name. We attempted to fix this, but it's a bit of a pain to do correctly, since there's no built-in support for merging proto messages in Go with a field mask (but there is for other languages).
See this issue for more info (trying not to have GH auto-reference it since it's closed):
https://github.com/mennanov/fieldmask-utils/pull/15
server/server.go
Outdated
Paths: []string{"details.build.provenance.built_artifacts"}, | ||
}}) | ||
|
||
log.Debug("UpdateOccurrence response", zap.Any("response", res)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I'd move this after the err
check, since this won't be useful if err != nil
as res
will be nil
anyways.
left := response.Occurrences[l].CreateTime.AsTime() | ||
right := response.Occurrences[r].CreateTime.AsTime() | ||
|
||
return left.Before(right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want earliest? Is most current the most valid? HA... Maybe it shouldn't happen but I bet we'd want the latest one.
Along with the work in #3, this closes #1
This is to account for workflows where an artifact is built and pushed to a registry in a lower environment and then retagged and pushed to a registry in a production environment. This is common at larger companies that want to restrict who can deploy production artifacts, but still want to allow for fast iteration in lower environments.
Add a build occurrence:
Update with the prod artifact:
If there are multiple occurrences that match the artifact, the oldest is used. We could potentially allow for multiple to updated if there's a use case for that.