Add Host ComputeAttributes, fix tests and add them to CI

[Fodoj]

Please tell if I need to split that in smaller ones - I will try to slice it in different branches inside my fork.

1. Management of usergroups - just simple datasource and resource for Usergroups;
2. Fix parameter bug - in current version, parameters drift is never detected, because the code requests the wrong key (in GET /api/hosts/:id params are inside parameters key, not host_parameters_attributes;
3. Add compute attributes - I kept it as hypervisor-unspecific as possible and only for simple flat attributes. I will submit more changes for tested compute attributes, for cases like vSphere volumes.

[Fodoj]

@lhw I pushed two more changes to this PR and updated the PR description. Probably in subsequent PR automatic tests should be fixed/re-enabled via Github Actions :-)

[lhw]

I didn't merge the changes yet because I'm still considering what to do about the wrapping of the json for some API calls but not others. You already found another one. The inconsistency in foremans API is highly annoying. I think we need a solid solution to this issue. Did you have any ideas @mgusek-interhyp? You also stumbled upon this issue.

And yes tests are really required at this point. Would make new PRs a lot quicker if we could instantly check for regressions etc.

[mgusek-interhyp]

The problem is, golang has no idea of optional arguments for a function. The golang 'way' is to define some wrapper functions: ` type FooOpts struct { // optional arguments Value string }

func NewFoo(mandatory string) { return NewFooWithOpts(mandatory, &FooOpts{}) }

func NewFooWithOpts(mandatory string, opts FooOpts) { // implement some empty checks defaultOpts := false // or just declare (var defaultOpts bool) to use zero // default of false. I just prefer to explicity set the value. if (opts) == FooOpts{} { // Assume that this was not provided and continue with default process defaultOpts = true }

// utilizing the optionals from this point would be below if !defaultOpts { fmt.Println(*opts.Value) } } ` or using an slice for all other options, like Fodoj's solution. But all items in a slice have to be the same type, so it's problematic to add a new option with an other type, is'nt it ? I like to follow the way of a programing language. I would prefer a WrapJsonWithLocation() or similar function.

[Fodoj]

@mgusek-interhyp totally agree that another function would be nicer and more Go-way. The reason I went with a slice is because then I don't have to modify too much code, especially in other resources - as the tests are not running, its hard to verify that I didnt break something :) But I can change it a new function for sure.

Would you then prefer this PR to have tests, or a separate PR? I am afraid this one is already pretty big, plus I have more changes in pipeline that I would prefer not to drop on top of this PR

[Fodoj]

@lhw @mgusek-interhyp I fixed the tests. Host tests are commented out in wayfair original and it appeared to be huge effort to make them work (and in general the tests code in this provider is.. curious). But at least now go test ./... returns 'ok' for all tests that are not commented out. I've also added GH Actions, so hopefully this PR is more mergeable now :)

[mgusek-interhyp]

There's a lot of changes, i would think @lhw prefers smaller PR's. And we have the problem with WrapJson(), lets fix that first, @lhw what do youi think ?

[Fodoj]

@mgusek-interhyp let me split them in smaller pieces then,

Adding owners is here: https://github.com/HanseMerkur/terraform-provider-foreman/pull/25

[Fodoj]

@mgusek-interhyp I've split WrapJSON() in this commit, in a different PR - https://github.com/HanseMerkur/terraform-provider-foreman/pull/26/commits/9e1876798bc314649c7036a260672292e7c7c2e1.

Once those two PRs are merged, I will rebase this one so that it only fixes tests and adds compute attributes. Or should I also split adding tests and compute attributes into two separate PRs?

[Fodoj]

@lhw @mgusek-interhyp one more change cut to PR: https://github.com/HanseMerkur/terraform-provider-foreman/pull/29. Would it be okay to keep this PR with both compute_attributes and tests inside?

[lhw]

@lhw @mgusek-interhyp one more change cut to PR: #29. Would it be okay to keep this PR with both compute_attributes and tests inside?

Honestly. You are doing a lot of work right now on the provider and I don't want to make it harder for you. And I have done the same before. Just keep it as is and after the rebase/merge I'll also tag a new release.

[Fodoj]

@lhw 👍 thanks a lot; at least now we have CI running, adds me some confidence in changes :-) Next PRs will be smaller.