Skip to content

Conversation

petemounce
Copy link
Collaborator

@petemounce petemounce commented Aug 27, 2020

Checklist
  • make test-all (UNIX) passes. CI will also test this
  • unit and/or integration tests are included (if applicable)
  • documentation is changed or added (if applicable)

Description 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 the Resource 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:

var resourceTypes = map[string]Resource{
  "addr": Addr{},
  "command": Command{},
  ...
  "user": User{},
}

func Resources() map[string]Resource {
  return resourceTypes
}

and similarly for Outputers (but where the Outputer interface grows a FormatOptions() []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 calls RegisterOutputer.

The pros, though:

  • no longer need the init entry per file (3 lines of code * 16)
  • no need for the RegisterResource or RegisterOutputer 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)
  • no need for the mutexes inside the of the Outputers() or FormatOptions()
  • since Outputer.FormatOptions() would now exist, IsValidFormatOption() becomes a map lookup

What 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.

@aelsabbahy
Copy link
Member

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

@petemounce
Copy link
Collaborator Author

@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) {
Copy link
Collaborator Author

@petemounce petemounce Sep 10, 2020

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?

if !(util.IsValueInList(opt, list)) {
list = append(list, opt)
}
var found map[string]*formatOption
Copy link
Collaborator Author

@petemounce petemounce Sep 10, 2020

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.

@petemounce
Copy link
Collaborator Author

@aelsabbahy what do you think with the refactor to maps?

@aelsabbahy
Copy link
Member

@ripienaar I would love your take on this. You're the only one I know who may be impacted by this.

@ripienaar
Copy link
Collaborator

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

@aelsabbahy aelsabbahy merged commit a15241d into goss-org:master Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants