-
Notifications
You must be signed in to change notification settings - Fork 318
Add support for managed clusters #137
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
Conversation
|
I would love to see more test coverage. |
|
@roidelapluie me too - I took a look and didn't see anything that was easy for me to base my tests off of. This field has similar semantics to "enabled", which is also untested. It is only usable in |
|
@bdimcheff you are correct. this is impossible to test. I had the same issues with enabled flag. You cannot event read the value, so you cannot test if it changed correctly. |
|
BTW: i suggest you remove the vendor update commit, update will be covered by #143 |
|
@matejvelikonja will do, thanks. Should I rebase it out or do you avoid force pushes to feature branches? |
243708b to
71e7d9f
Compare
|
ok this is just a single commit on top of #143 now so the diff should be nice once that's merged. |
|
|
||
| * `enabled` - (Optional, boolean) Determines if cluster is active or not. Defaults to `true`. | ||
|
|
||
| * `managed` - (Optional, boolean) Determines if cluster is managed by gitlab or not. Defaults to `true`. |
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.
Please make a comment that we can not read that value in the doc
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.
will do. FYI: enabled works this way too, so I'll add it there too.
| ImportState: true, | ||
| ImportStateVerify: true, | ||
| ImportStateVerifyIgnore: []string{"enabled", "kubernetes_token"}, | ||
| ImportStateVerifyIgnore: []string{"enabled", "kubernetes_token", "managed"}, |
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.
how hard is it to add a test that creates a cluster managed and one unmanaged, even if we can not read the status?
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.
ok I added a test but I'm not sure it really tests much
The gitlab api has a "managed" field to disable cluster management features when adding a kubernetes cluster. This implements that field. Default is true if you don't supply it. This can only be toggled at cluster creation time, so changes will delete and readd the integration. https://docs.gitlab.com/ee/api/project_clusters.html#add-existing-cluster-to-project
71e7d9f to
a50a368
Compare
The gitlab api has a "managed" field to disable cluster management features when adding a kubernetes cluster. This implements that field. Default is true if you don't supply it.
This can only be toggled at cluster creation time, so changes will delete and readd the integration.
https://docs.gitlab.com/ee/api/project_clusters.html#add-existing-cluster-to-project
This also upgrades go-gitlab to an unreleased version that includes this field, which may be problematic.
I've run this fork for a few days successfully, and it has behaved as I expect it to.