Skip to content

Commit f91115d

Browse files
MM-67180 Fixing issue with gitlab connect to instance not working with whitespaces (#631)
* fix: issue with gitlab connect to instance not working with whitespaces - Added Makefile fix for version bumping * Added instance command tests and trimming of instance names
1 parent eff0f61 commit f91115d

File tree

3 files changed

+146
-4
lines changed

3 files changed

+146
-4
lines changed

Makefile

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,12 @@ endif
4343
# Used for semver bumping
4444
PROTECTED_BRANCH := master
4545
APP_NAME := $(shell basename -s .git `git config --get remote.origin.url`)
46-
CURRENT_VERSION := $(shell git describe --abbrev=0 --tags)
46+
CURRENT_VERSION := $(strip $(shell git describe --abbrev=0 --tags))
47+
LATEST_RELEASE_TAG_RAW := $(shell git tag -l "v*" --sort=-v:refname | grep -v '\-rc' | head -n 1 || true)
48+
LATEST_RELEASE_TAG := $(strip $(LATEST_RELEASE_TAG_RAW))
49+
ifeq ($(LATEST_RELEASE_TAG),)
50+
LATEST_RELEASE_TAG := $(CURRENT_VERSION)
51+
endif
4752
VERSION_PARTS := $(subst ., ,$(subst v,,$(subst -rc, ,$(CURRENT_VERSION))))
4853
MAJOR := $(word 1,$(VERSION_PARTS))
4954
MINOR := $(word 2,$(VERSION_PARTS))
@@ -81,6 +86,11 @@ endef
8186
patch: ## to bump patch version (semver)
8287
$(call check_protected_branch)
8388
$(call check_pending_pulls)
89+
@$(eval BASE_VERSION := $(strip $(LATEST_RELEASE_TAG)))
90+
@$(eval BASE_VERSION_PARTS := $(subst ., ,$(subst v,,$(subst -rc, ,$(BASE_VERSION)))))
91+
@$(eval MAJOR := $(word 1,$(BASE_VERSION_PARTS)))
92+
@$(eval MINOR := $(word 2,$(BASE_VERSION_PARTS)))
93+
@$(eval PATCH := $(word 3,$(BASE_VERSION_PARTS)))
8494
@$(eval PATCH := $(shell echo $$(($(PATCH)+1))))
8595
$(call prompt_approval,$(MAJOR).$(MINOR).$(PATCH))
8696
@echo Bumping $(APP_NAME) to Patch version $(MAJOR).$(MINOR).$(PATCH)
@@ -91,6 +101,11 @@ patch: ## to bump patch version (semver)
91101
minor: ## to bump minor version (semver)
92102
$(call check_protected_branch)
93103
$(call check_pending_pulls)
104+
@$(eval BASE_VERSION := $(strip $(LATEST_RELEASE_TAG)))
105+
@$(eval BASE_VERSION_PARTS := $(subst ., ,$(subst v,,$(subst -rc, ,$(BASE_VERSION)))))
106+
@$(eval MAJOR := $(word 1,$(BASE_VERSION_PARTS)))
107+
@$(eval MINOR := $(word 2,$(BASE_VERSION_PARTS)))
108+
@$(eval PATCH := $(word 3,$(BASE_VERSION_PARTS)))
94109
@$(eval MINOR := $(shell echo $$(($(MINOR)+1))))
95110
@$(eval PATCH := 0)
96111
$(call prompt_approval,$(MAJOR).$(MINOR).$(PATCH))
@@ -102,6 +117,11 @@ minor: ## to bump minor version (semver)
102117
major: ## to bump major version (semver)
103118
$(call check_protected_branch)
104119
$(call check_pending_pulls)
120+
@$(eval BASE_VERSION := $(strip $(LATEST_RELEASE_TAG)))
121+
@$(eval BASE_VERSION_PARTS := $(subst ., ,$(subst v,,$(subst -rc, ,$(BASE_VERSION)))))
122+
@$(eval MAJOR := $(word 1,$(BASE_VERSION_PARTS)))
123+
@$(eval MINOR := $(word 2,$(BASE_VERSION_PARTS)))
124+
@$(eval PATCH := $(word 3,$(BASE_VERSION_PARTS)))
105125
$(eval MAJOR := $(shell echo $$(($(MAJOR)+1))))
106126
$(eval MINOR := 0)
107127
$(eval PATCH := 0)

server/command.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ func (p *Plugin) handleUnInstallInstance(args *model.CommandArgs, parameters []s
273273
return p.getCommandResponse(args, "Please specify the instance name.", true), nil
274274
}
275275

276-
instanceName := strings.Join(parameters, " ")
276+
instanceName := strings.TrimSpace(strings.Join(parameters, " "))
277277

278278
err := p.uninstallInstance(instanceName)
279279
if err != nil {
@@ -288,7 +288,7 @@ func (p *Plugin) handleSetDefaultInstance(args *model.CommandArgs, parameters []
288288
return p.getCommandResponse(args, "Please specify the instance name.", true), nil
289289
}
290290

291-
instanceName := strings.Join(parameters, " ")
291+
instanceName := strings.TrimSpace(strings.Join(parameters, " "))
292292
err := p.setDefaultInstance(instanceName)
293293
if err != nil {
294294
return p.getCommandResponse(args, err.Error(), true), nil
@@ -346,7 +346,7 @@ func (p *Plugin) handleConnect(args *model.CommandArgs, parameters []string) (*m
346346
}
347347

348348
// Set the default instance for the user before connecting
349-
instanceName := parameters[0]
349+
instanceName := strings.TrimSpace(strings.Join(parameters, " "))
350350
err := p.setDefaultInstance(instanceName)
351351
if err != nil {
352352
return p.getCommandResponse(args, err.Error(), true), nil

server/command_test.go

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -502,3 +502,125 @@ func TestAddWebhookCommand(t *testing.T) {
502502
})
503503
}
504504
}
505+
506+
type instanceNameTestCase struct {
507+
name string
508+
parameters []string
509+
instanceName string
510+
exists bool
511+
}
512+
513+
var instanceNameTestCases = []instanceNameTestCase{
514+
{"single word", []string{"SimpleInstance"}, "SimpleInstance", true},
515+
{"whitespace in name", []string{"Gitlab", "Test", "Instance"}, "Gitlab Test Instance", true},
516+
{"multiple whitespaces", []string{"My", "Corporate", "GitLab", "Server"}, "My Corporate GitLab Server", true},
517+
{"leading and trailing whitespaces", []string{" Test", "Instance", "Name "}, "Test Instance Name", true},
518+
{"non-existent instance", []string{"NonExistent", "Instance"}, "NonExistent Instance", false},
519+
}
520+
521+
func setupInstanceCommandTest(t *testing.T, instanceList []string, instanceConfig map[string]InstanceConfiguration) (*Plugin, *string, *plugintest.API) {
522+
t.Helper()
523+
p := new(Plugin)
524+
p.configuration = &configuration{EncryptionKey: testEncryptionKey}
525+
526+
instanceListJSON, _ := json.Marshal(instanceList)
527+
instanceConfigJSON, _ := json.Marshal(instanceConfig)
528+
529+
var capturedMessage string
530+
api := &plugintest.API{}
531+
api.On("KVGet", instanceConfigNameListKey).Return(instanceListJSON, nil)
532+
api.On("KVGet", instanceConfigMapKey).Return(instanceConfigJSON, nil)
533+
api.On("KVSetWithOptions", mock.AnythingOfType("string"), mock.Anything, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil)
534+
api.On("SavePluginConfig", mock.Anything).Return(nil)
535+
api.On("LogError", mock.Anything, mock.Anything, mock.Anything).Return(nil)
536+
537+
siteURL := "https://mattermost.example.com"
538+
conf := &model.Config{}
539+
conf.ServiceSettings.SiteURL = &siteURL
540+
api.On("GetConfig", mock.Anything).Return(conf)
541+
542+
api.On("SendEphemeralPost", mock.Anything, mock.MatchedBy(func(post *model.Post) bool {
543+
capturedMessage = post.Message
544+
return true
545+
})).Return(&model.Post{})
546+
547+
p.SetAPI(api)
548+
p.client = pluginapi.NewClient(api, p.Driver)
549+
550+
return p, &capturedMessage, api
551+
}
552+
553+
func TestInstanceCommands(t *testing.T) {
554+
args := &model.CommandArgs{UserId: "user_id", ChannelId: "channel_id"}
555+
556+
t.Run("set-default", func(t *testing.T) {
557+
for _, tc := range instanceNameTestCases {
558+
t.Run("set-default "+tc.name, func(t *testing.T) {
559+
instanceList := []string{tc.instanceName}
560+
if !tc.exists {
561+
instanceList = []string{"Other Instance"}
562+
}
563+
p, msg, _ := setupInstanceCommandTest(t, instanceList, nil)
564+
_, _ = p.handleSetDefaultInstance(args, tc.parameters)
565+
if tc.exists {
566+
assert.Contains(t, *msg, "Instance '"+tc.instanceName+"' has been set as the default.")
567+
} else {
568+
assert.Contains(t, *msg, "does not exist")
569+
}
570+
})
571+
}
572+
t.Run("no parameters", func(t *testing.T) {
573+
p, msg, _ := setupInstanceCommandTest(t, nil, nil)
574+
_, _ = p.handleSetDefaultInstance(args, []string{})
575+
assert.Contains(t, *msg, "Please specify the instance name.")
576+
})
577+
})
578+
579+
t.Run("uninstall", func(t *testing.T) {
580+
for _, tc := range instanceNameTestCases {
581+
t.Run("uninstall "+tc.name, func(t *testing.T) {
582+
instanceList := []string{tc.instanceName}
583+
config := map[string]InstanceConfiguration{tc.instanceName: {GitlabURL: "https://gitlab.example.com"}}
584+
if !tc.exists {
585+
instanceList = []string{"Other Instance"}
586+
config = map[string]InstanceConfiguration{"Other Instance": {GitlabURL: "https://gitlab.example.com"}}
587+
}
588+
p, msg, _ := setupInstanceCommandTest(t, instanceList, config)
589+
_, _ = p.handleUnInstallInstance(args, tc.parameters)
590+
if tc.exists {
591+
assert.Contains(t, *msg, "Instance '"+tc.instanceName+"' has been uninstalled.")
592+
} else {
593+
assert.Contains(t, *msg, "not found in the list")
594+
}
595+
})
596+
}
597+
t.Run("no parameters", func(t *testing.T) {
598+
p, msg, _ := setupInstanceCommandTest(t, nil, nil)
599+
_, _ = p.handleUnInstallInstance(args, []string{})
600+
assert.Contains(t, *msg, "Please specify the instance name.")
601+
})
602+
})
603+
604+
t.Run("connect", func(t *testing.T) {
605+
for _, tc := range instanceNameTestCases {
606+
t.Run("connect "+tc.name, func(t *testing.T) {
607+
instanceList := []string{tc.instanceName}
608+
if !tc.exists {
609+
instanceList = []string{"Other Instance"}
610+
}
611+
p, msg, _ := setupInstanceCommandTest(t, instanceList, nil)
612+
_, _ = p.handleConnect(args, tc.parameters)
613+
if tc.exists {
614+
assert.Contains(t, *msg, "Click here to link your GitLab account")
615+
} else {
616+
assert.Contains(t, *msg, "does not exist")
617+
}
618+
})
619+
}
620+
t.Run("no parameters", func(t *testing.T) {
621+
p, msg, _ := setupInstanceCommandTest(t, nil, nil)
622+
_, _ = p.handleConnect(args, []string{})
623+
assert.Contains(t, *msg, "Please specify the instance name.")
624+
})
625+
})
626+
}

0 commit comments

Comments
 (0)