feat(libartifact): Add NewArtifactStoreWithEventCallback for artifact events #418
Conversation
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6451 |
nimdrak
changed the title
Fixes #27260 Signed-off-by: ByoungUk Lee <[email protected]>
|
|
||
| var ErrEmptyArtifactName = errors.New("artifact name cannot be empty") | ||
|
|
||
| type EventCallback func(eventType, name, digest string, attrs map[string]string) |
There was a problem hiding this comment.
- Can the type use value use a Go ~enum with named constants? That would also provide a location to potentially document anything non-obvious about the types.
- Overall, some documentation of the values (and promises, or lack of promises, or constraints) would be valuable; in particular, the
attrsfield is vague enough to be basically not consumable by Go code. If this structure is required / preferred by Podman’s event system and documented there, please say at least that. - (The
eventTypeseems to be named “status” in the user.)
| return NewArtifactStoreWithEventCallback(storePath, sc, nil) | ||
| } | ||
|
|
||
| func NewArtifactStoreWithEventCallback(storePath string, sc *types.SystemContext, eventCallback EventCallback) (*ArtifactStore, error) { |
There was a problem hiding this comment.
This is correct as far as it goes, but adding a new function for every option combination does not scale.
Most importantly, do we need a new function at all? In general, c/common makes no API promises. Is there an extra stability promise that libartifact made?
If we don’t need to promise API stability, just adding a parameter to the existing function would work… or an options struct, collecting all the not-conceptually-essential options, so that we have named parameters.
If we do want API stability, we probably want a mechanism that also allows meaningfully reporting errors, and does not expose a public struct: the generic Go default seems to be a “functional options” mechanism, maybe similar to image/signature/sigstore.NewSigner and its Option.
3 participants
This commit introduces a new constructor,
NewArtifactStoreWithEventCallback, to allow external consumers to register a callback function forartifact-related events.The following methods now invoke the event callback if one is provided:
RemovePullPushAddExtractExtractTarStreamFixes #27260 containers/podman#27324