-
Notifications
You must be signed in to change notification settings - Fork 263
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
feat: Add support to combine service create --filename with other options #937
feat: Add support to combine service create --filename with other options #937
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsimansk: 0 warnings.
In response to this:
Description
Add enhancement to the command
kn service create -f
to accept other options to modify the created service.Per discussion in #923 we should decide how
name
is handled:
- Provided cmdline
name
overrides the name in the file- Keep the current implementation that if
name
is provided on cmdline it must match the name in the file.Changes
- Add support to combine service create --filename with other options
Reference
Fixes #923
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/test-infra repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
Thanks for contribution. Left couple comments. Please try and address.
docs/cmd/kn_service_create.md
Outdated
@@ -64,7 +64,7 @@ kn service create NAME --image IMAGE | |||
--concurrency-utilization int Percentage of concurrent requests utilization before scaling up. (default 70) | |||
-e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-). | |||
--env-from stringArray Add environment variables from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret:). Example: --env-from cm:myconfigmap or --env-from secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --env-from cm:myconfigmap-. | |||
-f, --filename string Create a service from file. | |||
-f, --filename string Create a service from file. The created service can be further modified by combining with other options.Example: -f /path/to/file --env NAME=value will also add environment variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"options.Example:" => "options, e.g.," OR "options. For example:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"also add environment variable" --> "also add an environment variable"
command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file.") | ||
command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file. "+ | ||
"The created service can be further modified by combining with other options."+ | ||
"Example: -f /path/to/file --env NAME=value will also add environment variable.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Use same resolution
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could comment these lines and all tests are still passing. So this tells me no test for errors when editFlags.Apply(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't editFlags.Apply()
already covered ? Ideally, there is a dedicated unit test only for this method, and should not be tested indirectly within tests that test that intend to test other stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I look at the overall test suite I see, that editFlags.Apply()
is already tested to a high degree. Looking at that method though it looks like that it deserves some refactoring, it is far too large.
@@ -303,12 +303,11 @@ func constructServiceFromFile(cmd *cobra.Command, editFlags ConfigurationEditFla | |||
// Set namespace in case it's specified as --namespace | |||
service.ObjectMeta.Namespace = namespace | |||
|
|||
// We need to generate revision to have --force replace working | |||
revName, err := servinglib.GenerateRevisionName(editFlags.RevisionName, &service) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this been removed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's superseded by combination of markFlagMakesRevision
and editFlags.Apply
, per my suggestion/proposal from previous PR.
https://github.com/knative/client/pull/937/files#diff-74b189b0b506d256ede4322475d950f5R285
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, and I think it makes sense.
Co-authored-by: Roland Huß <[email protected]>
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks !
/lgtm
/approve
[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 |
Description
Add enhancement to the command
kn service create -f
to accept other options to modify the created service.Per discussion in #923 we should decide how
name
is handled:name
overrides the name in the filename
is provided on cmdline it must match the name in the file.Changes
Reference
Fixes #923