Conversation
|
@dlom: This pull request references HIVE-3134 which is a valid jira issue. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
📝 WalkthroughWalkthroughThe change removes the direct dependency on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@dlom: This pull request references HIVE-3134 which is a valid jira issue. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2886 +/- ##
==========================================
+ Coverage 50.29% 50.32% +0.02%
==========================================
Files 280 280
Lines 34323 34337 +14
==========================================
+ Hits 17264 17281 +17
+ Misses 15698 15696 -2
+ Partials 1361 1360 -1
🚀 New features to boost your workflow:
|
2uasimojo
left a comment
2uasimojo
left a comment
There was a problem hiding this comment.
/test e2e
Not so sure about this one @dlom. I remember @lleshchi and I had to churn quite a bit to find a functional and elegant way to do this, and the hand-crafted json introspection you're doing is, no offense, only one of those things :P
I can be talked into it, but would like to explore other options first. See inline.
| // setJSONLabels sets the given labels on a JSON-encoded Kubernetes resource's metadata. | ||
| func setJSONLabels(jsonData []byte, newLabels map[string]string) ([]byte, error) { | ||
| var manifest map[string]interface{} | ||
| if err := json.Unmarshal(jsonData, &manifest); err != nil { | ||
| return nil, fmt.Errorf("error unmarshalling manifest JSON: %w", err) | ||
| } | ||
| metadata, _ := manifest["metadata"].(map[string]interface{}) | ||
| if metadata == nil { | ||
| metadata = map[string]interface{}{} | ||
| manifest["metadata"] = metadata | ||
| } | ||
| labels, _ := metadata["labels"].(map[string]interface{}) | ||
| if labels == nil { | ||
| labels = map[string]interface{}{} | ||
| metadata["labels"] = labels | ||
| } | ||
| for k, v := range newLabels { | ||
| labels[k] = v | ||
| } | ||
| return json.Marshal(manifest) | ||
| } |
There was a problem hiding this comment.
Hm. Is there a way to call an existing/already-imported upstream lib to decode into a metav1.Object on which we can SetLabels()? What you've done here should be pretty straightforward and stable, but it feels... dirty.
|
/test e2e another infra flake |
|
@dlom: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
3 participants
xref: HIVE-3134
This one warrants a test to make sure we don't lost any functionality. The dep was also pulling in a 16 KB png into our tree. Goodbye!
/assign @2uasimojo
Summary by CodeRabbit
Refactor
Tests
Chores