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

Checked cluster name before executing kubectl command #243

Merged
merged 3 commits into from Feb 6, 2020

Conversation

girishg4t
Copy link
Contributor

@girishg4t girishg4t commented Feb 5, 2020

This commit fix the bug 230 which shows redundant error message
when deployed in miticluster, added check for cluster name and
created new function to get cluster name from kubectl command

@girishg4t girishg4t requested a review from PrasadG193 Feb 5, 2020
result := GetClusterNameFromKubectlCmd(str)

if result != "minikube" {
t.Errorf("Expecte minikube but got %v ", result)
Copy link
Contributor

@sanketsudake sanketsudake Feb 5, 2020

Choose a reason for hiding this comment

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

expected instead of Expecte

Copy link
Contributor Author

@girishg4t girishg4t Feb 5, 2020

Choose a reason for hiding this comment

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

done

pkg/utils/utils_test.go Show resolved Hide resolved

//GetClusterNameFromKubectlCmd this will return cluster name from kubectl command
func GetClusterNameFromKubectlCmd(cmd string) string {
i := strings.Index(cmd, "--cluster-name") + 15 //15 indicates 15 letters in --cluster-name string with space or equal
Copy link
Contributor

@sanketsudake sanketsudake Feb 5, 2020

Choose a reason for hiding this comment

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

Please see if we could use valid regex instead.

Copy link
Collaborator

@PrasadG193 PrasadG193 Feb 5, 2020

Choose a reason for hiding this comment

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

Not as a part of this PR but we should use cli parser (like github.com/urfave/cli) to parse BotKube commands.

Copy link
Contributor Author

@girishg4t girishg4t Feb 5, 2020

Choose a reason for hiding this comment

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

added regex

@girishg4t girishg4t requested a review from sanketsudake Feb 5, 2020
@girishg4t girishg4t requested a review from sanketsudake Feb 6, 2020
Copy link
Collaborator

@PrasadG193 PrasadG193 left a comment

LGTM

r, _ := regexp.Compile(`--cluster-name[=|' ']([^\s]*)`)
//this gives 2 match with cluster name and without
matchedArray := r.FindStringSubmatch(cmd)
var s string = ""
Copy link
Collaborator

@PrasadG193 PrasadG193 Feb 6, 2020

Choose a reason for hiding this comment

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

Minor:

Suggested change
var s string = ""
var s string

Copy link
Contributor Author

@girishg4t girishg4t Feb 6, 2020

Choose a reason for hiding this comment

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

done

@girishg4t girishg4t requested a review from PrasadG193 Feb 6, 2020
@PrasadG193 PrasadG193 merged commit a75470a into develop Feb 6, 2020
2 checks passed
@PrasadG193 PrasadG193 deleted the bugfix/multicluster branch Apr 12, 2020
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.

None yet

3 participants