[prism] Add internal/config package #25406
Conversation
|
Looks like this depends on something not in the go.mod file yet. So while this is good to review, it shouldn't be merged until we get that green after #25404 is merged. |
|
R: @jrmccluskey |
|
Run Go PreCommit |
|
Run GoPortable PreCommit |
| return &Variant{parent: r, name: name, handlers: vs.Handlers} | ||
| } | ||
|
|
||
| type Variant struct { |
There was a problem hiding this comment.
Nit: I don't love the naming collision of the internal, little-v variant and the exported Variant, it doesn't read particularly clearly.
There was a problem hiding this comment.
Renamed variants to configFile, since it's logically the whole file.
Renamed variant to rawVariant, since it's the raw struct being decoded into by YAML.
| variantIDs, handerIDs []string | ||
| } | ||
|
|
||
| func NewHandlerwRegistry() *HandlerRegistry { |
There was a problem hiding this comment.
Is there a naming convention reason why this is NewHandlerwRegistry() instead of NewHandlerRegistry()?
There was a problem hiding this comment.
A typo. Whoops. Thanks. This isn't yet used, so it wasn't caught. Soon!
| // Variants returns the IDs of all registered variations. | ||
| func (r *HandlerRegistry) Variants() []string { | ||
| return r.variantIDs | ||
| } | ||
|
|
||
| // Handlers returns the IDs of all handlers used in variations. | ||
| func (r *HandlerRegistry) UsedHandlers() []string { | ||
| return r.handerIDs | ||
| } | ||
|
|
||
| // GetVariant returns the Variant witn the given name. | ||
| // If none exist, GetVariant returns nil. | ||
| func (r *HandlerRegistry) GetVariant(name string) *Variant { | ||
| vs, ok := r.variations[name] | ||
| if !ok { | ||
| return nil | ||
| } | ||
| return &Variant{parent: r, name: name, handlers: vs.Handlers} | ||
| } |
There was a problem hiding this comment.
Nit: could we move these to be contiguous with the rest of the HandlerRegistry fns?
|
PTAL. Reordered the types & methods in top down order where definitions are involved. This puts HandlerRegistry at the bottom. Added missing exported type comments, as well as the renames. |
|
PS. These are exactly the kind of comments I'm looking for at this stage of Prism. |
|
Run Go PreCommit |
|
Thanks! Gonna wait until the automated jobs pick up. It seems there's a bit of a Queue at present. |
|
https://ci-beam.apache.org/job/beam_PreCommit_Go_Phrase/226/console This was the run for this PR after the most recent commit. Merging. Thanks for the quick reviews! Now it gets fun. |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Add internal/config package, which is the core for configuring the Runner with variants and handlers.
Uber PR for all changes: #25391
See #24789
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.