OCPBUGS-74543: expose: Fix labels not being added to route #2189
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-74543, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
WalkthroughEncapsulated route customization in a new finalizeRoute method on ExposeOptions that sets Route.Spec.Host, Route.Spec.Path, Route.Spec.WildcardPolicy and parses/merges user-provided labels into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tchap: This pull request references Jira Issue OCPBUGS-74543, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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
bot
requested review from
ardaguclu,
ingvagabund and
zhouying7780
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cli/expose/expose.go`:
- Around line 188-205: The code unconditionally overwrites
route.ObjectMeta.Labels with the local variable labels (which is nil when
o.ExposeServiceOptions.Labels is omitted), discarding labels copied by
route.UnsecuredRoute; change the code to only assign route.ObjectMeta.Labels
when o.ExposeServiceOptions.Labels is non-empty (i.e., after parsing via
generate.ParseLabels or by checking len(o.ExposeServiceOptions.Labels) > 0) so
existing service labels from route.UnsecuredRoute are preserved when --labels is
not provided.
tchap
force-pushed
the
expose-service-use-labels
branch
2 times, most recently
from
b02b779 to
d49d3a9
Compare
|
I don't mind adding an e2e test for this once the OTE tests are merged. |
tchap
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
tchap
changed the title
tchap
force-pushed
the
expose-service-use-labels
branch
from
d49d3a9 to
d5afaa8
Compare
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@tchap: This pull request references Jira Issue OCPBUGS-74543, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/test e2e-aws-ovn-serial-2of2 |
|
/test e2e-agnostic-ovn-cmd |
tchap
force-pushed
the
expose-service-use-labels
branch
from
d5afaa8 to
022f13d
Compare
tchap
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Code looks good to me. Some trivial comments for tests.
That would be great |
tchap
force-pushed
the
expose-service-use-labels
branch
from
022f13d to
377cb34
Compare
|
I was then thinking the change is a bit too small to spend time on e2e tests, so I added the unit tests. |
|
Thanks |
openshift-ci
bot
added
lgtm
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cli/expose/expose_test.go`:
- Around line 97-99: The test currently calls err.Error() directly which can
panic if err is nil; change the comparison to safely derive errMsg (e.g., set
errMsg := "" if err == nil else err.Error()) and then compare tc.expectedErrMsg
to errMsg (referencing tc.expectedErrMsg, err, errMsg and the test function in
expose_test.go) so no nil pointer dereference occurs during t.Errorf.
The --labels option was being ignored when exposing a service. This is now fixed and .metadata.labels is set to the desired value.
tchap
force-pushed
the
expose-service-use-labels
branch
from
377cb34 to
08d5f27
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, tchap 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 |
|
@tchap: 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. |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@tchap: This PR has been marked as verified by 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. |
|
@tchap: Jira Issue Verification Checks: Jira Issue OCPBUGS-74543 Jira Issue OCPBUGS-74543 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
4 participants
The
--labelsoption was being ignored when exposing a service.This is now fixed and .metadata.labels is set to the desired value.