Add kubernetes_role resource #226
Conversation
Add a new resource to allow the management of `role` resources in Terraform
ghost
added
the
There was a problem hiding this comment.
Hello and thank you so much for this work!
Really appreciate the effort.
I took a quick look over and made some spot comments mostly about getting the code to adhere to few new standards we're trying to follow.
Please have a look at my comments and let know if I can help out in any way.
| Required: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, | ||
| }, |
There was a problem hiding this comment.
There seems to be a missing attribute.
Is there any reason why non_resource_urls isn't present here?
There was a problem hiding this comment.
NonResourceUrls appear to be only for ClusterRoleBindings, despite what the Kubernetes API suggests: kubernetes/kubernetes#30924 (comment)
There was a problem hiding this comment.
Oh, wasn't aware of that twist to the story. It does appear in the API definition.
Then nevermind, leave it out.
| Rules: rules, | ||
| } | ||
|
|
||
| out, err := conn.RbacV1().Roles(namespace).Update(&role) |
There was a problem hiding this comment.
The Update operation of client-go actually does a whole-object replace.
This is a bit of a mismatch with Terraform's definition of the Update action, which is to do in-place updates. In client-go the equivalent operation would be a Patch.
Whenever these are not possible, Terraform has the ForceNew option, which makes everything more explicit.
What I mean to suggest is that we either implement the Update function using Patch operations or don't implement an Update operation at all and rely on ForceNew behaviour which delete the old version of the resource before creating a new one.
|
|
||
| log.Printf("[INFO] Role %s deleted", name) | ||
|
|
||
| d.SetId("") |
There was a problem hiding this comment.
This is not needed. The Delete operation will do this automatically.
| log.Printf("[INFO] Checking role %s", name) | ||
| _, err = conn.RbacV1().Roles(namespace).Get(name, meta_v1.GetOptions{}) | ||
| if err != nil { | ||
| if statusErr, ok := err.(*errors.StatusError); ok && statusErr.ErrStatus.Code == 404 { |
There was a problem hiding this comment.
The client-go SDK has helpers for determining the kind of error that was received.
Can you please refactor this line to make use of those instead of checking directly against 404?
See the example from StatefulSets which I mentioned before.
The exhaustive list is here: https://godoc.org/k8s.io/apimachinery/pkg/api/errors
| return true, err | ||
| } | ||
|
|
||
| func expandRules(rules []interface{}) []v1.PolicyRule { |
There was a problem hiding this comment.
We are trying to standardise on returning pointers to complex types instead of values across the provider.
Would you mind aligning the expand* functions to that pattern?
Here's a refactoring PR for reference: #231
website/docs/r/role.html.markdown
Outdated
| #### Attributes | ||
|
|
||
| * `generation` - A sequence number representing a specific generation of the desired state. | ||
| * `resource_version` - An opaque value that represents the internal version of this replication controller that can be used by clients to determine when replication controller has changed. For more info see [Kubernetes reference](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#concurrency-control-and-consistency) |
There was a problem hiding this comment.
This paragraph also mentions a Replication Controlller.
website/docs/r/role.html.markdown
Outdated
|
|
||
| The following arguments are supported: | ||
|
|
||
| * `metadata` - (Required) Standard replication controller's metadata. For more info see [Kubernetes reference](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#metadata) |
There was a problem hiding this comment.
This paragraph mentions a Replication Controlller.
Copy/Paste oversight?
website/docs/r/role.html.markdown
Outdated
|
|
||
| #### Arguments | ||
|
|
||
| * `annotations` - (Optional) An unstructured key value map stored with the replication controller that may be used to store arbitrary metadata. For more info see [Kubernetes reference](http://kubernetes.io/docs/user-guide/annotations) |
There was a problem hiding this comment.
This paragraph also mentions a Replication Controlller.
website/docs/r/role.html.markdown
Outdated
|
|
||
| * `annotations` - (Optional) An unstructured key value map stored with the replication controller that may be used to store arbitrary metadata. For more info see [Kubernetes reference](http://kubernetes.io/docs/user-guide/annotations) | ||
| * `generate_name` - (Optional) Prefix, used by the server, to generate a unique name ONLY IF the `name` field has not been provided. This value will also be combined with a unique suffix. For more info see [Kubernetes reference](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#idempotency) | ||
| * `labels` - (Optional) Map of string keys and values that can be used to organize and categorize (scope and select) the replication controller. **Must match `selector`**. For more info see [Kubernetes reference](http://kubernetes.io/docs/user-guide/labels) |
There was a problem hiding this comment.
Next 3 paragraphs also mentions a Replication Controlller.
website/docs/r/role.html.markdown
Outdated
| #### Attributes | ||
|
|
||
| * `generation` - A sequence number representing a specific generation of the desired state. | ||
| * `resource_version` - An opaque value that represents the internal version of this replication controller that can be used by clients to determine when replication controller has changed. For more info see [Kubernetes reference](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#concurrency-control-and-consistency) |
There was a problem hiding this comment.
This paragraph also mentions a Replication Controlller.
|
The changes I've pushed should fix all of the issues mentioned |
|
@alexsomesan could you re-review and merge this and #235 if everything is fine? These two additions would help me massively with a project I'm working on :) |
|
@alexsomesan Any further feedback on this PR? |
|
@alexsomesan Any feedback on this PR? |
|
@wjam Thanks for making those changes. I'll now schedule a full CI run of the acceptance tests and get back to you with the results. |
Add a new resource to allow the management of
roleresources in Terraform