-
Notifications
You must be signed in to change notification settings - Fork 486
Add ability to runtime list / introspect resources #611
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
Add ability to runtime list / introspect resources #611
Conversation
I agree with your observation, the simple map seems like the better solution. The whole init/register thing is great if goss was being used as a library and we want to allow users to register their custom types, but that doesn't matter since the genny stuff won't work. The outputs can be dynamically added without anything else breaking. So, changing that would drop that "feature".. but.. it's not exactly an advertised feature. I say switch to simple maps and see if anyone complains |
@aelsabbahy cool - I'll change this branch so that it moves to that pattern for both outputers and resources. |
) | ||
|
||
func RegisterOutputer(name string, outputer Outputer, formatOptions []string) { | ||
func RegisterOutputer(name string, outputer Outputer) { |
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.
@aelsabbahy I kept the method, but note the breaking change. Obviously, I can keep the previous signature and just ignore the param if you'd rather - or something else entirely?
outputs/outputs.go
Outdated
if !(util.IsValueInList(opt, list)) { | ||
list = append(list, opt) | ||
} | ||
var found map[string]*formatOption |
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.
Using a map guarantees set-semantics on the option.name
.
I'm not concerned by the looping because we have less than 10 outputers.
d55c7a8
to
eb6a9bd
Compare
d8eea60
to
7901578
Compare
@aelsabbahy what do you think with the refactor to maps? |
32b2b96
to
8af3edc
Compare
@ripienaar I would love your take on this. You're the only one I know who may be impacted by this. |
I would like to create a registry of all known types and eventually have the ability to register new resources on the fly when embedded, so this seems like a tiny step in the right direction - getting where I want is a giant undertaking, maybe over the holidays end of year hehe |
Checklist
make test-all
(UNIX) passes. CI will also test thisDescription of change
Inside #607 (comment) I made a hack to allow me to iterate over the different types of resources.
This PR introduces the same pattern to resources as exists already for outputs.
I spent an afternoon trying to come up with a way to cover this in a unit test that would start to fail if a new resource type was ever added, but didn't come up with something. I tried iterating over the exported structs inside of the
resources
package, but failed to find a way of checking whether they implemented theResource
interface; golang doesn't maintain a type registry, so this was some pretty gnarly reflection.Having made that attempt, I actually now think it would be simpler to do something like:
and similarly for
Outputers
(but where theOutputer
interface grows aFormatOptions() []string
that returns a constant set.The cons are that to add a new one, you have to remember add an entry to the map. I think this is no worse than the current setup, where you must remember to make the
init
that callsRegisterOutputer
.The pros, though:
RegisterResource
orRegisterOutputer
funcs, or mutexes, because the maps are already statically defined and externally, they're read-only via the getter func (unless I've misunderstood how by-value/by-ref works, of course, it wouldn't be the first time)Outputers()
orFormatOptions()
Outputer.FormatOptions()
would now exist,IsValidFormatOption()
becomes a map lookupWhat do you think? I'd be happy to make a separate PR to change to this approach for
Outputer
, so you can see it, and if you like that more, that could merge and then I could adapt this one.