-
Notifications
You must be signed in to change notification settings - Fork 42
Conversation
This PR is not fixing #192. |
@vpnachev , the current fix is able to remove bastion resources include (ec2 , sg, 22 rules) manually if there any resource leaking. in #192 do you mean I need to refactor |
My idea is The implementation is, of course cloud dependent because on some providers, e.g. Azure, a public IP is directly attached to the VM and a bastion is not used. |
sign-off
|
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, the PR looks very good. Just a few minor things in the comments. Can you please also rebase the PR
?
pkg/cmd/ssh.go
Outdated
return nil | ||
}, | ||
} | ||
|
||
cmd.Flags().BoolVar(&bastionVM, "j", false, "Use Bastion VM as jump box") |
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.
Please replace by
cmd.Flags().BoolVarP(&bastionVM, "jumpbox", "j", false, "Use Bastion VM as jump box")
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 am using local flags that seem not to support it.
too many arguments in call to cmd.Flags().BoolVar
have (*bool, string, string, bool, string)
want (*bool, string, bool, string)go
pkg/cmd/ssh_aws.go
Outdated
@@ -64,15 +64,21 @@ func sshToAWSNode(nodeName, path, user, pathSSKeypair string, sshPublicKey []byt | |||
a.createBastionHostSecurityGroup() | |||
fmt.Println("") | |||
|
|||
// if global flag -c is specified call cleanupAwsBastionHost |
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 would rather name the flag cleanup
to make things more clear. Maybe we can also document it in the readme. We only have this for aws
. In my opinion it would also make sense to add this for the other infrastructures, what do you think?
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.
sure.
due to -c already use by no-cache, I change to -e as erase
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 agree with you, regarding the other infrastructures. I will work on that.
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.
Readme add
pkg/cmd/ssh.go
Outdated
return nil | ||
}, | ||
} | ||
|
||
cmd.Flags().BoolVar(&bastionVM, "j", false, "Use Bastion VM as jump box") |
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 am using local flags that seem not to support it.
too many arguments in call to cmd.Flags().BoolVar
have (*bool, string, string, bool, string)
want (*bool, string, bool, string)go
What this PR does / why we need it:
instance-status.status,Values=ok
then ssh (ssh timeout due to status check initiating)gardenctl ssh xxxx -c
Which issue(s) this PR fixes:
Fixes #191
Fixes #192
Special notes for your reviewer:
global flags testing
gardenctl ssh (any hostname) -c
gardenctl ssh and exit
Release note: