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

Add a command-line option to edit status of resource with specified yaml file #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sergenyalcin
Copy link

Usage: kubectl edit-status <resource> <resource-name> --namespace <namespace> --in <filepath>

@sergenyalcin
Copy link
Author

sergenyalcin commented Feb 26, 2021

Hi @ulucinar,

If you are interested in this feature, the PR can be tested with the following steps:

  • kubectl -n <namespace> get <resource> <resource_name> -o yaml > <filepath>

  • Edit status subresource of resource from yaml output it is located in ) by using an editor (vi, vscode, notepad++, etc.)

  • kubectl edit-status <resource> <resource-name> --namespace <namespace> --in <filepath>

@ulucinar ulucinar self-requested a review May 1, 2021 13:50
Copy link
Owner

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Hi @sergenyalcin,
Thank you very much for the PR!

Let's define what we expect in the yaml file passed with the new -i option. In my opinion, it should be possible just to specify a partial status section in input file. What do you think?

My suggestion would be as follows:

  • If -i option is specified, branch out and call the new EditStatusOptions.writeResourceStatusFromInputFile instead of EditStatusOptions.editResource. However, execution had better continue from rest of EditStatusOptions.Run in order to prevent code duplication. Although at the moment we just call EditStatusOptions.writeResourceStatus, we may do some other post-processing later.
  • Then in EditStatusOptions.writeResourceStatusFromInputFile, we could just read and replace the status JSON object into the temp file and the rest stays the same. So instead of opening an interactive editor for the user to edit the status section, we would be replacing it from the specified file with the -i option.

What do you think? And again, thank you very much for your contribution. It's very much appreciated.

Also could you please sign your commit with -S option of git commit? We can setup a call if you need help in doing so.

Comment on lines +257 to +261
if err = o.writeResourceStatusFromInputFile(); err != nil {
return
}

return nil
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if err = o.writeResourceStatusFromInputFile(); err != nil {
return
}
return nil
return o.writeResourceStatusFromInputFile()


defer func() {
if errClose := f.Close(); err != nil {
err = errClose
Copy link
Owner

Choose a reason for hiding this comment

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

We will not be returning the IO error from File.Close from writeResourceStatusFromInputFile. In order for this to work, you should be assigning the error in the deferred call to a named return value. For an example, please see the deferred call in EditStatusOptions.Run.

Comment on lines +395 to +399
if err = o.writeResourceStatus(f); err != nil {
return err
}

return nil
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if err = o.writeResourceStatus(f); err != nil {
return err
}
return nil
return o.writeResourceStatus(f)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants