Skip to content

Conversation

dsimansk
Copy link
Contributor

Fixes #15891

Proposed Changes

  • Fix labels and annotations propagation to k8s service on update

Release Note

Fix labels and annotations propagation to k8s service on update

/cc @dprotaso @Fedosin

At least a few tests would be useful here. I'll add those based on initial feedback.

@knative-prow knative-prow bot requested review from Fedosin and dprotaso May 16, 2025 07:41
@knative-prow knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 16, 2025
@dprotaso
Copy link
Member

test failures look legit

@dprotaso
Copy link
Member

This behaviour should have been captured in conformance tests as per the spec - https://github.com/knative/specs/blob/main/specs/serving/knative-api-specification-1.0.md#service-1

Want to add a test here - https://github.com/knative/serving/tree/main/test/conformance/api/v1

Copy link

codecov bot commented May 21, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 80.90%. Comparing base (bbf34f6) to head (96f85e6).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
pkg/testing/v1/service.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15892      +/-   ##
==========================================
- Coverage   80.93%   80.90%   -0.04%     
==========================================
  Files         210      210              
  Lines       16731    16778      +47     
==========================================
+ Hits        13542    13574      +32     
- Misses       2839     2852      +13     
- Partials      350      352       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dsimansk
Copy link
Contributor Author

/test istio-latest-no-mesh

@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 21, 2025
@dsimansk
Copy link
Contributor Author

/test istio-latest-no-mesh-tls

@dsimansk
Copy link
Contributor Author

dsimansk commented May 22, 2025

test failures look legit

Fixed that to preserve existing labels and annotations on k8s service.

This behaviour should have been captured in conformance tests as per the spec - https://github.com/knative/specs/blob/main/specs/serving/knative-api-specification-1.0.md#service-1

Want to add a test here - https://github.com/knative/serving/tree/main/test/conformance/api/v1

@dprotaso thanks for hints. The spec doesn't explicitly mention Kubernetes Service and labels propagation. Should we amend that to the docs? I'll take a look at adding conformance tests to cover it.

@dprotaso
Copy link
Member

@dsimansk it's already in the spec

Updates to [Service] spec, metadata.labels, and metadata.annotations of the Service MUST be copied to the appropriate Configuration or Route,

So we just need to update conformance

@knative-prow knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 27, 2025
@dsimansk dsimansk force-pushed the pr/fix-labels branch 3 times, most recently from 450af3f to 3a48db5 Compare May 28, 2025 10:41
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 28, 2025
@dsimansk
Copy link
Contributor Author

@dprotaso @Fedosin this should be ready for next round. I've simplified the change check per suggestions, thanks. And added conformance tests for annotations (extended existing) and labels.

@dsimansk dsimansk added the kind/bug Categorizes issue or PR as related to a bug. label May 29, 2025
Copy link
Contributor

@Fedosin Fedosin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2025
Copy link

knative-prow bot commented May 29, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, Fedosin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot merged commit 6d6d25e into knative:main May 29, 2025
68 checks passed
@dsimansk dsimansk deleted the pr/fix-labels branch May 29, 2025 07:54
@dsimansk
Copy link
Contributor Author

@dprotaso any objection to cherry-pick to 1.18 and 1.17 release branches?

@dprotaso
Copy link
Member

dprotaso commented Jun 2, 2025

@dsimansk nope cherry pick away

@dsimansk
Copy link
Contributor Author

dsimansk commented Jun 3, 2025

/cherry-pick release-1.18
/cherry-pick release-1.17

@knative-prow-robot
Copy link
Contributor

@dsimansk: cannot checkout release-v1.17: error checking out "release-v1.17": exit status 1 error: pathspec 'release-v1.17' did not match any file(s) known to git

In response to this:

/cherry-pick release-v1.18
/cherry-pick release-v1.17

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-sigs/prow repository.

@knative-prow-robot
Copy link
Contributor

@dsimansk: new pull request created: #15907

In response to this:

/cherry-pick release-1.18
/cherry-pick release-1.17

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-sigs/prow repository.

@knative-prow-robot
Copy link
Contributor

@dsimansk: cannot checkout release-v1.18: error checking out "release-v1.18": exit status 1 error: pathspec 'release-v1.18' did not match any file(s) known to git

In response to this:

/cherry-pick release-v1.18
/cherry-pick release-v1.17

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-sigs/prow repository.

@knative-prow-robot
Copy link
Contributor

@dsimansk: new pull request created: #15908

In response to this:

/cherry-pick release-1.18
/cherry-pick release-1.17

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Knative Service labels and annotations are not propagated to k8s service on update

4 participants