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

Rename --concurrency-target and --concurrency-utilization #1490

Merged
merged 2 commits into from
Oct 27, 2021
Merged

Rename --concurrency-target and --concurrency-utilization #1490

merged 2 commits into from
Oct 27, 2021

Conversation

rhuss
Copy link
Contributor

@rhuss rhuss commented Oct 26, 2021 •
edited
Loading

The new option names are --scale-target and --scale-utilization
Both options are still available but deprecated. Reasoning is that both
options really belong to the 'scale' option group as they influencen
the scaling behaviour.

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 26, 2021
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 26, 2021
@rhuss rhuss changed the title Rename --concurrency-target and --concurrency-limit to --scale-target and --scale-limit, respectively Rename --concurrency-target and --concurrency-limit Oct 26, 2021
@rhuss rhuss changed the title Rename --concurrency-target and --concurrency-limit Rename --concurrency-target and --concurrency-utilization Oct 26, 2021
@rhuss rhuss force-pushed the pr/rename-concurrency-options branch from 8448054 to ff0b99d Compare October 26, 2021 08:20
@codecov
Copy link

codecov bot commented Oct 26, 2021 •
edited
Loading

Codecov Report

Merging #1490 (06a723b) into main (03045c8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1490      +/-   ##
==========================================
+ Coverage   79.15%   79.17%   +0.01%     
==========================================
  Files         162      162              
  Lines        8445     8451       +6     
==========================================
+ Hits         6685     6691       +6     
  Misses       1077     1077              
  Partials      683      683
Impacted Files Coverage Δ
pkg/serving/config_changes.go 89.85% <ø> (ø)
...kg/kn/commands/service/configuration_edit_flags.go 80.24% <100.00%> (+0.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03045c8...06a723b. Read the comment docs.

@rhuss
Copy link
Contributor Author

rhuss commented Oct 26, 2021

The changelog has not been updated yet, waiting on #1489 to avoid conflicts.

But ready to review ;-) // cc. @dsimansk

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021
@dsimansk
Copy link
Contributor

@rhuss looks good. As you mentioned the rebase is needed with the previous renaming PR, otherwise OK.

@rhuss
Rename --concurrency-target and --concurrency-utilization
18a5e8d 
The new option names are --scale-target and --scale-limit, respectively

Both options are still available but deprecated. Reasoning is that both
options really belong to the 'scale' option group as they influencen
the scaling behaviour.
@rhuss
rebased and added changelog
06a723b
@rhuss rhuss force-pushed the pr/rename-concurrency-options branch from ff0b99d to 06a723b Compare October 26, 2021 09:27
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021
@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 86.8% 87.1% 0.3

@dsimansk
Copy link
Contributor

/approve
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, 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:

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 3366619 into knative:main Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dsimansk dsimansk

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rhuss @dsimansk @knative-metrics-robot @knative-prow-robot