purpleidea / mgmt

Next generation distributed, event-driven, parallel config management!
https://purpleidea.com/tags/mgmtconfig/
GNU General Public License v3.0
3.58k stars 311 forks source link

Add a test for PR # 685 #686

Open purpleidea opened 2 years ago

purpleidea commented 2 years ago

https://github.com/purpleidea/mgmt/pull/685#issuecomment-1023139028

Would be great to add a simple test for this commit and also to double check that the test fails if that commit is reverted.

Commit: https://github.com/purpleidea/mgmt/commit/bf7e45439b044d18d0fdf3151fbdd7994875edce

Thanks to @frebib for the nice patch!

rafiramadhana commented 2 years ago

@purpleidea Hi. I can help with this, I am aware of what should be tested in regards to commit bf7e454. However, I still don't know what does the term kind means inside the StructKindToFieldNameTypeMap(kind string) (map[string]*types.Type, error). Can you help to pinpoint me some examples of how this struct kind looks? Maybe something like testdata? Thank you.

rafiramadhana commented 2 years ago

@purpleidea Hi again. After playing around with the StructKindToFieldNameTypeMap method using tests under the ./engine folder, it looks like to test the StructKindToFieldNameTypeMap, we should have a registered resource in the registeredResources map. Do we already have the convention to conveniently register a resource when doing tests? However, I found this TestRes struct. But this struct can't be imported in ./engine/util as it will result to an import cycle. Wdyt? Thanks.

purpleidea commented 2 years ago

@JefMasereel can probably point to the issue he found that this patch fixed... After our memory is refreshed, we can use TestRes (and import it elsewhere) or do something different. Does this help?

purpleidea commented 2 years ago

Original issue is here actually: https://github.com/purpleidea/mgmt/issues/684

rafiramadhana commented 2 years ago

@purpleidea Ok, I will take a look at #684 first.

Anyway, by "...our memory is refreshed", do you mean clearing the registeredResources?

I think we can create a new file of ./engine/util/test.go, then copy the implementation of TestRes (and its related dependencies) in ./engine/resource/test.go to that new file. Although it looks like a duplication, I think it is tolearble, considering that:

Wdyt? Thanks.

rafiramadhana commented 2 years ago

@purpleidea Oh ya, what does the term kind in this project means ya? I am quite new to this domain. Could you provide me some articles to read? Thank you.

purpleidea commented 2 years ago

@neverbeenthisweeb Sure!

Kind has a few uses, but in terms of the resources in mgmt, kind is file, svc, pkg, virt, etc... In other words the "type" of resource, although we don't use the word type here for various reasons.

purpleidea commented 2 years ago

Oh, and for articles: https://github.com/purpleidea/mgmt/blob/master/docs/on-the-web.md

You might want to read the oldest one by me, and maybe the first language one. There are some videos of conference talks you can watch if you're interested. Newer ones are more current and more useful.

JefMasereel commented 2 years ago

The Hetzner resource that I'm working on was failing type unification due to a circular type structure in one of the hcloud-go values that I store in a private resource field. @frebib added this patch to skip type checks for unexported fields, which solved the issues I was having. purpleidea has listed all the relevant issues by now, not sure if I can help beyond that at the moment.

rafiramadhana commented 2 years ago

@purpleidea @JefMasereel Got it, I will try to grasp those information first.

Thanks for the help guys.

ofekatr commented 1 year ago

I see that the implementation was revised in aff6331: https://github.com/purpleidea/mgmt/blob/aff633121199e666c70a147831114846e1baa29e/engine/util/util.go#L279-L283

Is this issue still relevant? I've opened a PR, a review would be appreciated :shipit:

https://github.com/purpleidea/mgmt/pull/709

cc @JefMasereel @purpleidea

purpleidea commented 1 year ago

@ofekatr Thank you for the PR, I sent an initial review. If you're looking for some mgmt patches to write, ping me and happy to guide you. Cheers!