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.

Sign up for GitHub

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

Jump to bottom

Added back functionality to deprecated max-scale/min-scale #1010

Merged
merged 2 commits into from
Oct 6, 2020
Merged

Added back functionality to deprecated max-scale/min-scale #1010

merged 2 commits into from
Oct 6, 2020

Conversation

mpetason
Copy link
Contributor

Description

Adds back functionality for max-scale/min-scale options. I ended up swapping over naming but never added additional sections to update.

Changes

  • Adds back functionality for max-scale/min-scale

Reference

Fixes #1008

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 14, 2020
@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 14, 2020
@mpetason
Copy link
Contributor Author

Issue also mentions that I need to add e2e tests, which will take me a little bit longer to address.

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/kn/commands/service/configuration_edit_flags.go 83.0% 81.0% -2.0

maximilien
maximilien approved these changes Sep 14, 2020
View reviewed changes
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

/retest

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2020
@rhuss
Copy link
Contributor

rhuss commented Sep 15, 2020

/retest

rhuss
rhuss reviewed Sep 15, 2020
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

thanks for the quick fix ! As soon as we have this merged we should trigger a new minor release asap.

pkg/kn/commands/service/configuration_edit_flags.go Outdated
@@ -313,6 +313,22 @@ func (p *ConfigurationEditFlags) Apply(
}
}

// Deprecated option
if cmd.Flags().Changed("min-scale") {
Copy link
Contributor

Choose a reason for hiding this comment

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

you could add it to the if below with an "or", to avoid duplicated code. Also, I would add an comment for which release we can remove this option.

According to our policy this would be n+2 where n is the first version where this feature was deprecated (so 0.19 iiur)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestions. Went ahead and make the updates.

@rhuss
Copy link
Contributor

rhuss commented Sep 15, 2020

Issue also mentions that I need to add e2e tests, which will take me a little bit longer to address.

It's totally fine to have this quick fix first (so that we can deliver a patch release asap) and then add the e2e test later. This should also not be that hard as it is just to add some extra variations for these options.

@rhuss
Copy link
Contributor

rhuss commented Sep 15, 2020

/retest

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

/retest

1 similar comment
@mpetason
Copy link
Contributor Author

/retest

@mpetason
Copy link
Contributor Author

Finally passed the pull-knative-client-integration-tests test!

@rhuss
Copy link
Contributor

rhuss commented Sep 29, 2020

Thanks !

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2020
@navidshaikh
Copy link
Collaborator

The build tests are failing complaining markdown link check failure from test-infra scripts

scripts/test-infra/README.md
	ERROR	../kntest/pkg/kubetest2/gke/README.md
		Stat scripts/kntest/pkg/kubetest2/gke/README.md: no such file or directory

cc: @coryrc @chizhg

@navidshaikh
Copy link
Collaborator

@mpetason : you might want to try rebasing onto current master and see if the tests are through ?

@rhuss
Copy link
Contributor

rhuss commented Sep 29, 2020

Oh, the build issue comes from an update of the test scripts, coming in with a pr that was merged just before.

I will take care.

@rhuss
Copy link
Contributor

rhuss commented Sep 29, 2020

Infratest failure is because of a flake.

@mpetason
Copy link
Contributor Author

mpetason commented Oct 1, 2020

Sounds good, let me know if you end up needing me to do something on my end.

@rhuss
Copy link
Contributor

rhuss commented Oct 5, 2020

/retest

1 similar comment
@rhuss
Copy link
Contributor

rhuss commented Oct 5, 2020

/retest

@navidshaikh navidshaikh added the backport/candidate Consider this PR to be backported to the release branch label Oct 6, 2020
@navidshaikh
Copy link
Collaborator

The markdown check failure only happens in this PR but not others, @mpetason a rebase against latest master should help I think.

@mpetason
Copy link
Contributor Author

mpetason commented Oct 6, 2020

Sounds good, I'll get that taken care of.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020
@mpetason
Copy link
Contributor Author

mpetason commented Oct 6, 2020

Awesome. I think that fixed it. Thanks for the help and suggestions. I got some help and didn't follow the git recommendations and was able to rebase without adding 100 more commits haha.

navidshaikh
navidshaikh approved these changes Oct 6, 2020
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh 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-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, mpetason, navidshaikh, rhuss

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:
  • OWNERS [maximilien,navidshaikh,rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 0997cb0 into knative:master Oct 6, 2020
navidshaikh pushed a commit to navidshaikh/client that referenced this pull request Oct 7, 2020
@navidshaikh
Added back functionality to deprecated max-scale/min-scale (knative#1010
9c5c522 
)

* Added back functionality to deprecated max-scale/min-scale

* Updated based on review, removed duplicate code and added an or statement, also updated deprecated message
@navidshaikh navidshaikh mentioned this pull request Oct 7, 2020
Merged
knative-prow-robot pushed a commit that referenced this pull request Oct 7, 2020
@navidshaikh
[0.17] Backport 1044 and 10101 (#1048)
26eaf7e 
* Added back functionality to deprecated max-scale/min-scale (#1010)

* Added back functionality to deprecated max-scale/min-scale

* Updated based on review, removed duplicate code and added an or statement, also updated deprecated message

* fix(e2e): Let the subscription and related resource reconcile (#1044)

sleep for 5 seconds after subscription create and update in e2e

* Update CHANGELOG

Co-authored-by: Mike Petersen <[email protected]>
@navidshaikh navidshaikh added backported-to/0.17 and removed backport/candidate Consider this PR to be backported to the release branch labels Oct 12, 2020
rhuss pushed a commit to rhuss/knative-client that referenced this pull request Oct 12, 2020
@rhuss
Added back functionality to deprecated max-scale/min-scale (knative#1010
97e27d9 
)

* Added back functionality to deprecated max-scale/min-scale

* Updated based on review, removed duplicate code and added an or statement, also updated deprecated message
dsimansk added a commit to dsimansk/client that referenced this pull request Apr 7, 2022
@dsimansk
[release-v1.0.0] Update kn-plugin-func (knative#1010)
7f79c58 
* [release-v1.0.0] Update kn-plugin-func

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

@rhuss rhuss rhuss left review comments

@navidshaikh navidshaikh navidshaikh approved these changes

@maximilien maximilien maximilien approved these changes

@evankanderson evankanderson Awaiting requested review from evankanderson

Assignees

@rhuss rhuss

@navidshaikh navidshaikh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--min-scale/max-scale are broken
7 participants
@mpetason @knative-metrics-robot @rhuss @navidshaikh @knative-prow-robot @maximilien @googlebot