Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't default Revision values when BYO name is unchanged. #13565

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

evankanderson
Copy link
Member

Fixes #11512

Proposed Changes

  • If updating a Configuration using BYO revision names, do not apply Revision defaulting (as the attempt to create / update the Revision will fail with a "revisions are immutable" error.

I wasn't entirely sure how we wanted to handle the fact that the ConfigurationSpec could be embedded in a Service or a Configuration. If we'd prefer, I can smuggle it into and out of the context, which would make it easier for other projects to interact with Configuration validation. I did the current approach first because it was simpler.

Release Note

* Changes to Pod or Revision-level defaults during Knative upgrades will no longer be attempted (and failed) when supplying your own Revision name.

@knative-prow knative-prow bot added area/API API objects and controllers size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 31, 2022
@knative-prow knative-prow bot requested review from julz and vagababov December 31, 2022 06:14
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 31, 2022
@evankanderson
Copy link
Member Author

/assign @dprotaso @psschwei

@codecov
Copy link

codecov bot commented Dec 31, 2022

Codecov Report

Base: 86.47% // Head: 86.16% // Decreases project coverage by -0.31% ⚠️

Coverage data is based on head (f185cbe) compared to base (aaa992e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13565      +/-   ##
==========================================
- Coverage   86.47%   86.16%   -0.31%     
==========================================
  Files         196      197       +1     
  Lines       14600    14672      +72     
==========================================
+ Hits        12625    12642      +17     
- Misses       1676     1730      +54     
- Partials      299      300       +1     
Impacted Files Coverage Δ
pkg/apis/serving/v1/configuration_defaults.go 100.00% <100.00%> (ø)
pkg/apis/serving/v1/service_defaults.go 100.00% <100.00%> (ø)
pkg/reconciler/revision/background.go 90.90% <0.00%> (-1.82%) ⬇️
pkg/autoscaler/scaling/multiscaler.go 87.24% <0.00%> (-1.35%) ⬇️
cmd/autoscaler/main.go 9.47% <0.00%> (-0.42%) ⬇️
cmd/autoscaler/leaderelection.go 0.00% <0.00%> (ø)
pkg/autoscaler/statserver/server.go 77.37% <0.00%> (+1.45%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@evankanderson
Copy link
Member Author

I switched to using a Context-passed *ConfigurationSpec, as it seemed more helpful for anyone who happens to be extending our APIs.

@evankanderson
Copy link
Member Author

(All three kourier and the contour test failed on cluster setup downloading the set of Istio releases...)

@evankanderson
Copy link
Member Author

@dprotaso , @psschwei -- it would be nice to get this in so that I can get #13398 in for the 1.9 release

@psschwei
Copy link
Contributor

/retest

@evankanderson
Copy link
Member Author

(yay, tests pass)

@evankanderson
Copy link
Member Author

(Reminder, this is blocking #13398 )

Copy link
Member

@dprotaso dprotaso left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold for minor nit

pkg/apis/serving/v1/configuration_defaults.go Outdated Show resolved Hide resolved
@knative-prow knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jan 14, 2023
@knative-prow
Copy link

knative-prow bot commented Jan 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, evankanderson

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 removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2023
@dprotaso
Copy link
Member

/hold cancel
/lgtm

@knative-prow knative-prow bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 18, 2023
@knative-prow knative-prow bot merged commit 2338826 into knative:main Jan 18, 2023
skonto pushed a commit to skonto/serving that referenced this pull request Jan 19, 2023
)

* Don't default Revision values when BYO name is unchanged.

Fixes knative#11512

* Switch to using a context-passed ConfigurationSpec reference instead of hard-coding Configuration and Service.

* Complete docstring for WithConfigurationSpec

* Update naming per dave's feedback
openshift-merge-robot pushed a commit to openshift-knative/serving that referenced this pull request Jan 19, 2023
) (#131)

* Don't default Revision values when BYO name is unchanged.

Fixes knative#11512

* Switch to using a context-passed ConfigurationSpec reference instead of hard-coding Configuration and Service.

* Complete docstring for WithConfigurationSpec

* Update naming per dave's feedback

Co-authored-by: Evan Anderson <[email protected]>
skonto added a commit to skonto/serving that referenced this pull request Jan 20, 2023
skonto added a commit to skonto/serving that referenced this pull request Jan 20, 2023
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/knative-serving that referenced this pull request Mar 8, 2023
)

* Don't default Revision values when BYO name is unchanged.

Fixes knative#11512

* Switch to using a context-passed ConfigurationSpec reference instead of hard-coding Configuration and Service.

* Complete docstring for WithConfigurationSpec

* Update naming per dave's feedback
openshift-merge-robot pushed a commit to openshift-knative/serving that referenced this pull request Mar 10, 2023
) (#205)

* Don't default Revision values when BYO name is unchanged.

Fixes knative#11512

* Switch to using a context-passed ConfigurationSpec reference instead of hard-coding Configuration and Service.

* Complete docstring for WithConfigurationSpec

* Update naming per dave's feedback

Co-authored-by: Evan Anderson <[email protected]>
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. area/API API objects and controllers lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Our webhooks when introducing new defaults have issues with BYO revision name
3 participants