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

Improvements made in kubectl create secret generic help command #120337

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ritikaa96
Copy link
Contributor

@Ritikaa96 Ritikaa96 commented Sep 1, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds Details in --type flag description, Example for empty secret and added details in description

Which issue(s) this PR fixes:

Fixes #kubernetes/kubectl#1452

Does this PR introduce a user-facing change?

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

Added improvements in the --type flag description and more details in the output of `kubectl create secret generip help`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

…secret generic command

Signed-off-by: Ritikaa96 <ritika@india.nec.com>
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 1, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 1, 2023
@Ritikaa96
Copy link
Contributor Author

/sig cli

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 1, 2023
@Ritikaa96
Copy link
Contributor Author

I'd love to know if there are any suggestions for improvement for this PR. PTAL

@@ -69,6 +69,8 @@ var (

secretForGenericLong = templates.LongDesc(i18n.T(`
Create a secret based on a file, directory, or specified literal value.

Using generic subcommand indicate an Opaque secret. When used with empty --type flag, the command creates an empty secret of type Opaque.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little unclear on what this is trying to convey exactly but what I read it as is:

The default type for a secret is Opaque unless otherwise specified by the --type flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's right

@@ -143,7 +148,7 @@ func NewCmdCreateSecretGeneric(f cmdutil.Factory, ioStreams genericiooptions.IOS
cmd := &cobra.Command{
Use: "generic NAME [--type=string] [--from-file=[key=]source] [--from-literal=key1=value1] [--dry-run=server|client|none]",
DisableFlagsInUseLine: true,
Short: i18n.T("Create a secret from a local file, directory, or literal value"),
Short: i18n.T("Create a secret from a local file, directory, or literal value. Using generic subcommand indicate an Opaque secret. When used with empty --type flag, the command creates an empty secret of type Opaque."),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think similarly this should read something like:

The default type for a secret is Opaque unless otherwise specified by the --type flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes , i think this is more neat . I'll change the wordings

Copy link
Contributor Author

@Ritikaa96 Ritikaa96 Nov 29, 2023

Choose a reason for hiding this comment

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

for clarification not only when not specified but also when we use generic in the create command, the secret type will be opaque

For eg: kubectl create secret generic empty-secret

Copy link
Contributor

Choose a reason for hiding this comment

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

Right yeah because this is for the generic subcommand, so that should be fine. I think we don't need to specify that the generic subcommand is required for this in the help text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i tried without generic and the secret didn't get created. there was an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to mention generic as when I tried without it it showed

$ kubectl create secret my-empty
error: unknown command "my-empty"
See 'kubectl create secret -h' for help and examples

Copy link
Contributor

Choose a reason for hiding this comment

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

So this help text is nested within the generic subcommand and therefore it is not necessary to mention the command since it is already being passed into the cli by the user, so they are aware that this if for the generic subcommand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they are aware then there is no issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I think I'm being unclear, I think the help text here should read The default type for a secret is Opaque unless otherwise specified by the --type flag. because there's no need to specify that the help text pertains to the generic subcommand, since the user has already typed the subcommand, so they should know that the help text applies to the subcommand and there's no need to restate that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh. Got it. I made the changes already. Please take a look

@@ -161,7 +166,7 @@ func NewCmdCreateSecretGeneric(f cmdutil.Factory, ioStreams genericiooptions.IOS
cmd.Flags().StringSliceVar(&o.FileSources, "from-file", o.FileSources, "Key files can be specified using their file path, in which case a default name will be given to them, or optionally with a name and file path, in which case the given name will be used. Specifying a directory will iterate each named file in the directory that is a valid secret key.")
cmd.Flags().StringArrayVar(&o.LiteralSources, "from-literal", o.LiteralSources, "Specify a key and literal value to insert in secret (i.e. mykey=somevalue)")
cmd.Flags().StringSliceVar(&o.EnvFileSources, "from-env-file", o.EnvFileSources, "Specify the path to a file to read lines of key=val pairs to create a secret.")
cmd.Flags().StringVar(&o.Type, "type", o.Type, i18n.T("The type of secret to create"))
cmd.Flags().StringVar(&o.Type, "type", o.Type, i18n.T("The string value indicate type of secret which is used to facilitate programmatic handling of secret data. Type can be built-in or user defined and vary in terms of validations performed and constraints kubernetes imposed on them. Empty type value indicate Opaque secret. Learn more about built-in types and usage here : https://kubernetes.io/docs/concepts/configuration/secret/#secret-types "))
Copy link
Contributor

Choose a reason for hiding this comment

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

This specifically I think needs to be improved by a code change. I see that the fact that the default secret type being Opaque is inherited from the corev1 Secrets type somehow. I think that this should be indicated by properly using the default input for the StringVar function, which here is simply set using o.Type (a string), but here, because in the NewSecretOptions does not define this field in the SecretOptions type, the help text only displays that the default value will be an empty string, so I propose that we change what is returned from NewSecretOptions to add the Opaque type as the default return. That way the cobra/pflags help texts render those values automatically and clearly, then we update this line to simply read:

The type of the secret to create. For more information see https://kubernetes.io/docs/concepts/configuration/secret/#secret-types

Or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so what do you suggest ? should I do the code change in the same PR?
I'll need to investigate more and raise another PR if that's what we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll also add this suggested change which is in full description for now
in next commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think a subsequent PR to make it so the auto-generated help text is accurate would be cleaner. If you need help with the code change PR feel free to reach out to me with any questions and I'll try my best to help answer them or find someone who knows the answer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks a lot . i will investigate and raise the PR. this one though will remain seperate

@Ritikaa96
Copy link
Contributor Author

HI @mpuckett159 .
I have made the suggested changes.
PTAL!! and thanks for the suggestions.

@Ritikaa96
Copy link
Contributor Author

Hi, i see the merge is blocked due to release note labels, how do we resolve this? i don't think i can add release note none label by myself. Please Let me know.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 25, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 25, 2024
@Ritikaa96
Copy link
Contributor Author

Hi, i see the merge is blocked due to release note labels, how do we resolve this? i don't think i can add release note none label by myself. Please Let me know.

Hi @mpuckett159 Please suggest something regarding this

@Ritikaa96
Copy link
Contributor Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 27, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label May 27, 2024
@mpuckett159
Copy link
Contributor

Hi, please add a release note instead of removing the label. This edits what the user will see, so this requires a description that can be included in the general release notes.

@Ritikaa96
Copy link
Contributor Author

Ritikaa96 commented May 30, 2024

@mpuckett159 Thanks for clarifying. I'll add the release note .

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 30, 2024
@Ritikaa96
Copy link
Contributor Author

@mpuckett159 Added the release note . PTAL

Copy link
Contributor

@mpuckett159 mpuckett159 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 8f7df8bb4386f68c3b26c8a326c2319ae2c54adb

@mpuckett159
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mpuckett159, Ritikaa96
Once this PR has been reviewed and has the lgtm label, please assign ardaguclu for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@Ritikaa96
Copy link
Contributor Author

/assign @ardaguclu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

None yet

5 participants