WINC-650: [wmco] Apply worker role to BYOH Nodes #541
Conversation
Moves everything that was present in the annotations package into the metadata package. This is being done as we will need to use the functions present in the annotations package for labels as well, and it makes more sense to generalize the functions considering the similarities between annotations and labels.
openshift-ci
bot
added
do-not-merge/work-in-progress
Refactors the patch creation in order to enable better unit testing. Specifically, this commit makes it so that metadata.generatePatch no longer returns the marshalled patches, but instead returns the slice of patches it generated, allowing those to be marshalled at the call sites, metadata.GenerateAddPatch() and metadata.GenerateRemovePatch(). This commit is important for two reasons. Firstly, using assert.elementsMatch to test that a byte slice is correct does not seem like the right thing to do. This seems as if it just tests that the chars are all the same, so the patchdata could be made out of order and this unit test would not catch that. Secondly this allows for more complex testing, which will be required in a future commit where generatePatch() handles both annotations and labels. Additionally, since the tests had to be changed, I changed one case to exercise the escaping functionality, something that was being missed by the tests.
sebsoto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/approve cancel |
openshift-ci
bot
removed
the
approved
| // GenerateRemovePatch creates a comma-separated list of operations to remove all given labels and annotations from an | ||
| // object. A "remove" patch fails transactionally if any of the annotations do not exist. | ||
| func GenerateRemovePatch(labels, annotations []string) ([]byte, error) { | ||
| labelMap := make(map[string]string) |
There was a problem hiding this comment.
can be renamed, as to not give away the data structure
There was a problem hiding this comment.
Not sure what I'd rename it to considering there is already a labels
pkg/metadata/metadata.go
Outdated
| // generatePatch creates a patch applying the given operation onto each given annotation key and value | ||
| func generatePatch(op string, labels, annotations map[string]string) ([]*patch.JSONPatch, error) { | ||
| if len(labels) == 0 && len(annotations) == 0 { | ||
| return nil, errors.New("cannot have both label and annotation maps empty") |
There was a problem hiding this comment.
labels and annotations empty will suffice.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aravindhp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Extends the annotation patch creation functions to allow for labels as well.
The machine-api-controller does this for Nodes associated with Machines, this needed to be done for BYOH Nodes as well.
|
Thanks for working on this @sebsoto |
|
/hold cancel |
|
/cherry-pick community-4.8 |
|
@sebsoto: new pull request created: #557 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.8 |
|
@mansikulkarni96: new pull request created: #597 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The goal of this PR is to ensure that BYOH Nodes have the worker role label.
In order to achieve this changes had to be made to the patch generation functions,
refactoring that makes up the bulk of the commits in the PR.