Update flag names to --request and --limit #872
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: navidshaikh 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 |
knative-prow-robot
added
approved
- Use singular flag names and support comma separated or repeated flag values - Update tests and docs
navidshaikh
force-pushed
the
pr/singular-resource-flag-master
branch
2 times, most recently
from
82eff15 to
8556521
Compare
navidshaikh
force-pushed
the
pr/singular-resource-flag-master
branch
from
8556521 to
6f403b4
Compare
navidshaikh
added
the
backport/candidate
CHANGELOG.adoc
Outdated
| | Update flag names to --request and --limit | ||
| | https://github.com/knative/client/pull/872[#872] | ||
| |=== | ||
|
|
There was a problem hiding this comment.
I think we can remove the v0.15.0 header as this version has never been officially released.
docs/cmd/kn_service_create.md
Outdated
| --limits string The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. | ||
| --limits-cpu string DEPRECATED: please use --limits instead. The limits on the requested CPU (e.g., 1000m). | ||
| --limits-memory string DEPRECATED: please use --limits instead. The limits on the requested memory (e.g., 1024Mi). | ||
| --limit strings The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. |
There was a problem hiding this comment.
why is the type here still string and not stringArray as for label ?
There was a problem hiding this comment.
if we use StringSliceVar type flags, we get strings (see traffic flags) and
if we use StringArrayVar type flags, we get stringArray
|
It's possible that labels use an array whereas limits/requests use a slice. I think we should later adapt labels etc. you slices, too. + we should think about how we can share code better to avoid that kind of differences in the future. |
Yes (see traffic flags)
Yes, we'd need to fix this for rest of the flags to work as mentioned in the examples of convention at https://github.com/knative/client/blob/master/conventions/cli.md#maps |
|
The following is the coverage report on the affected files.
|
navidshaikh
removed
the
backport/candidate
|
/lgtm |
* Update flag names to --request and --limit - Use singular flag names and support comma separated or repeated flag values - Update tests and docs * Update CHANGELOG * Update the flag description and examples * Remove release 0.15.0 header from CHANGELOG
| } | ||
| for _, c := range cases { | ||
| options := &ResourceOptions{} | ||
| options.Requests = c.requests | ||
| options.Limits = c.limits | ||
| err := options.Validate() | ||
| reqToRemove, limToRemove, err := options.Validate() |
There was a problem hiding this comment.
Maybe don't call it validate as the main purpose is no to extract the values. What's about options.Extract() or options.Parse() (or options.ParseAndValidate()) ?
There was a problem hiding this comment.
Sure, will update the name when we unify the other similar flags.
* Update flag names to --request and --limit - Use singular flag names and support comma separated or repeated flag values - Update tests and docs * Update CHANGELOG * Update the flag description and examples * Remove release 0.15.0 header from CHANGELOG
Co-authored-by: David Simansky <[email protected]>
Description