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

Create a common PodFlags for both Service command and ContainerSource command #935

Closed
daisy-ycguo opened this issue Jul 16, 2020 · 2 comments · Fixed by #943
Closed

Create a common PodFlags for both Service command and ContainerSource command #935

daisy-ycguo opened this issue Jul 16, 2020 · 2 comments · Fixed by #943
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature New feature or request
Milestone

Comments

@daisy-ycguo
Copy link
Member

daisy-ycguo commented Jul 16, 2020

The specification of knative Service and eventing ContainerSource include common flags to describe the pod which will be created. I propose to create a common type under pkg/kn/commands/flags for these flags.

The specification of knative Service includes a corev1.PodSpec, while ContainerSource includes a corev1.PodTemplateSpec. corev1.PodTemplateSpec includes corev1.PodSpec. So both Service and ContainerSource commands could accept a dozen of common flags for create command and update command.

In order to provide a unique interface and to leverage the existing code base, I propose to create a common PodFlags under pkg/kn/commands/flags and move some flags in pkg/kn/commands/service/configuration_edit_flags.go there. Then this PodFlags can be used in both Service commands and ContainerSource commands.

I think below attributes in ConfigurationEditFlags are common to both Service and ContainerSource.

Image   uniqueStringArg
Env     []string
EnvFrom []string
Mount   []string
Volume  []string

Command string
Arg     []string

RequestsFlags, LimitsFlags ResourceFlags // TODO: Flag marked deprecated in release v0.15.0, remove in release v0.18.0
Resources                  knflags.ResourceOptions
Scale                      int
MinScale                   int
MaxScale                   int
ConcurrencyTarget          int
ConcurrencyLimit           int
ConcurrencyUtilization     int
AutoscaleWindow            string
Port                       string
Labels                     []string
LabelsService              []string
LabelsRevision             []string
NamePrefix                 string
<del>RevisionName               string</del>
ServiceAccountName         string
ImagePullSecrets           string
Annotations                []string
ClusterLocal               bool
User                       int64

// Preferences about how to do the action.
<del>LockToDigest         bool</del>
<del>GenerateRevisionName bool</del>
<del>ForceCreate          bool</del>

Filename string

// Bookkeeping
<del>flags []string</del>
@daisy-ycguo
Copy link
Member Author

daisy-ycguo commented Jul 16, 2020

@rhuss @navidshaikh let me know your opinion to this proposed change.

@navidshaikh
Copy link
Collaborator

+1 for grouping the related flags together.

How about bringing PodFlags in pkg/kn/flags ?
Overall, the flag definitions are scattered in client repo: pkg/kn/flags, pkg/kn/commands/flags, pkg/kn/commads/, pkg/kn/commands/human_readable_flags.go. We can work on refactoring the flags defined in deeper pkg levels and bring them to pkg/kn/flags. This way its consistent for consumption within client and plugins.
Also, there are flags defined for properties like ResourceOptions (in pkg/kn/flags/resources.go), we could consider encapsulating them in PodFlags, this way the targeted utils like func (o *ResourceOptions) Validate() could be useful.

@navidshaikh navidshaikh added kind/feature New feature or request kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 5, 2020
@navidshaikh navidshaikh added this to the v0.17.0 milestone Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants