-
Notifications
You must be signed in to change notification settings - Fork 81
ROLE: openshift-patch-resource - patch a k8s/openshift resource #386
ROLE: openshift-patch-resource - patch a k8s/openshift resource #386
Conversation
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.
Overall looks good ... a few minor comments on the README
| api_version | The api_version of the resource | yes || | ||
| definition | The patch definition | yes || | ||
| kind | The kind of the resource | yes || | ||
| merge_type | A list of merge types ("strategic-merge, merge") | no | strategic-merge | |
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.
Can we add a link to the ansible k8s module in the defaults column (maybe as just a link when clicking strategic-merge
) to indicate that the default is coming from the actual module (i.e.: not set by this role).
Requirements | ||
------------ | ||
|
||
This role requires the `kubernetes` python module |
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.
Should we also mentioned a "valid" or "active" session to an OpenShift (or k8s) cluster as a requirement?
| name | The name of the resource | yes || | ||
| namespace | The namespace of the resource | no || | ||
|
||
Example Playbook |
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.
Could we include some example vars here as well? ... or maybe just point to the tests to see some examples?
Thank you @oybed, updated the README based on your feedback |
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.
LGTM
What does this PR do?
Let's you patch a k8s/openshift resource with either
strategic-merge
ormerge
strategyHow should this be manually tested?
Is there a relevant Issue open for this?
Provide a link to any open issues that describe the problem you are solving.
Use
resolves #<pr-number>
to auto-close at merge timeOther Relevant info, PRs, etc.
Please provide link to other PRs that may be related (blocking, resolves, etc.)
Who would you like to review this?
cc: @redhat-cop/casl