SREP-711: Add non-interactive command input to ssm command#716
Conversation
|
@MateSaary: This pull request references SREP-711 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
6d8dfaa to
863a364
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #716 +/- ##
==========================================
+ Coverage 50.71% 50.80% +0.09%
==========================================
Files 76 76
Lines 5843 5862 +19
==========================================
+ Hits 2963 2978 +15
- Misses 2475 2479 +4
Partials 405 405
🚀 New features to boost your workflow:
|
|
@MateSaary: This pull request references SREP-711 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@MateSaary: This pull request references SREP-711 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@MateSaary: This pull request references SREP-711 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
cmd/ocm-backplane/cloud/ssm.go
Outdated
| for i, arg := range args { | ||
| if arg == "--" { | ||
| if i+1 < len(args) { | ||
| return args[i+1:] |
There was a problem hiding this comment.
We might not need to do this by our own.
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02
Guideline 10:
The first -- argument that is not an option-argument should be accepted as a delimiter indicating the end of options. Any following arguments should be treated as operands, even if they begin with the '-' character.
The backplane elevate command is doing something similar, can use it as reference:
An example given by GenAI:
// main.go
package main
import (
"fmt"
"os"
"os/exec"
"strings"
"github.com/spf13/cobra"
)
func main() {
rootCmd := &cobra.Command{Use: "mycli"}
rootCmd.AddCommand(echoCmd())
if err := rootCmd.Execute(); err != nil {
os.Exit(1)
}
}
func echoCmd() *cobra.Command {
var shout bool
cmd := &cobra.Command{
Use: "echo",
Short: "Pass arguments to the echo command",
Run: func(cmd *cobra.Command, args []string) {
fmt.Println("Flag --shout:", shout)
fmt.Println("Args to echo:", args)
if shout {
for i, v := range args {
args[i] = strings.ToUpper(v)
}
}
// Run echo with args
out, err := exec.Command("echo", args...).CombinedOutput()
if err != nil {
fmt.Println("Error:", err)
return
}
fmt.Println("Output:", string(out))
},
}
cmd.Flags().BoolVar(&shout, "shout", false, "Uppercase all args")
return cmd
}
To run it:
go mod init mycli
go mod tidy
go run main.go echo --shout -- hello world
Flag --shout: true
Args to echo: [hello world]
Output: HELLO WORLD
Hope this helps. Let me know if I miss anything. Thanks!
There was a problem hiding this comment.
Great point, thanks! Did want to bring one thing to attention though before refactoring as such (might simply be me misusing/misunderstanding something)...
I refactored the code to take the above into account, and re-encountered the same issue I had previously not paid much attention to, and was the reason behind the parseArgsWithDoubleDash() function in the first place:
$ ocm-backplane cloud ssm -v debug --node <some-node> -- free -m
...
INFO[0011] Non-interactive command to be executed: debug free -m
...
It seems to cause issues if we add a parameter to the command prior, and this seems to be repeated in the elevate command:
$ ocmbp elevate -v debug "REASON" -- get pods
error: unknown command "REASON" for "oc"
ERRO[0000] exit status 1
Of course, in both cases, if we remove the preceding -v debug, the command works as normal. Is this expected behaviour or a flaw in this approach?
There was a problem hiding this comment.
Thank you for pointing out!
I think this behavior is caused by
backplane-cli/pkg/cli/globalflags/logs.go
Line 48 in b19177a
The -v flag has a default value info, so it doesn't take the following argument as its value. We will need an equal sign to pass a verbose level:
ocm backplane elevate -v=debug "test" -- get nodes
This behavior is from #139 . Probably we will need to revisit the -v behavior. But in this PR, let's assume the users will use -v=debug, so the debug won't be taken as a normal argument.
|
Hi @MateSaary , thank you very much for contributing to backplane-cli ! And thank you for keeping unit test in mind! I left some comments in line, could you help a look and share your thoughts? |
863a364 to
1f3067b
Compare
738efd1 to
197cb49
Compare
|
@MateSaary: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
Hi @feichashao thank you (again!) for your input, PTAL at the changes made now to simplify the additions significantly (in terms of maintainability), let me know if there's anything further to tweak 😄 |
|
/label tide/merge-method-squash |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feichashao, MateSaary The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
What this PR does / Why we need it?
This PR improves upon the
cloud ssmcommand by allowing non-interactive command execution within the SSM session through the--separator.The PR also removes some minor redundant unit test code variables.
Which Jira/Github issue(s) does this PR fix?
Special notes for your reviewer
Unit Test Coverage
Guidelines
Test coverage checks
Pre-checks (if applicable)