Repository: preslavmihaylov/todocheck
Pull request #47 (2020-08-27 01:49:45)

Validate origin format based on issue tracker
Solves #25

srafi1: I'm not sure how to validate the origin for Jira or Redmine, but this should validate the others properly
srafi1: Thanks for the comments. I fixed everything you mentioned, but I have a couple of questions. Why does `Validate()` return a list of errors when it can only ever have one according to the current logic? Also, when I ran the tests with `go test ./testing -v`, many of the tests failed. I ended up running my own tests using `go test ./testing -run Origins -v` and although they pass, I'm not sure if I'm running tests properly if the other ones are failing on my computer.
preslavmihaylov: > Thanks for the comments. I fixed everything you mentioned, but I have a couple of questions. Why does `Validate()` return a list of errors when it can only ever have one according to the current logic? Also, when I ran the tests with `go test ./testing -v`, many of the tests failed. I ended up running my own tests using `go test ./testing -run Origins -v` and although they pass, I'm not sure if I'm running tests properly if the other ones are failing on my computer.

The tests all pass when I run them from latest master:
![image](https://user-images.githubusercontent.com/7642330/92203389-bc947e80-ee89-11ea-955d-f5c43acc5e80.png)

Could you try running the test suite from latest master & see if they pass?

As for the `Validate` function, the goal is to batch report all issues with the configuration. 

Currently, your validation is incompatible with the rest but as more validations are added, there will be independent issues \w configurations which can be reported to the user.
preslavmihaylov: Blocked by #54 
preslavmihaylov: Hey, the PR blocking this one is now merged. Please pull latest master & continue from there (to enable the CI checks).
srafi1: I figured out the testing issue on my end, added a bunch of tests for the regex, and fixed the regex according to your comments. LMK if there's anything else to add.

Patch:
diff --git a/config/config.go b/config/config.go
index 7f3489c..3beb0ae 100644
--- a/config/config.go
+++ b/config/config.go
@@ -40,6 +40,14 @@ var (
 	gitRemoteOriginPattern     = regexp.MustCompile(`(?Um)url\s=\s\w+(://|@)(?P<origin>(?P<host>.+)?(:|/).+)(\.git)?$`)
 )
 
+var originPatterns = map[IssueTracker]*regexp.Regexp{
+	IssueTrackerJira:    regexp.MustCompile(`^(https?://)?[a-zA-Z0-9\-]+(\.[a-zA-Z0-9]+)+(:[0-9]+)?$`),
+	IssueTrackerGithub:  regexp.MustCompile(`^(https?://)?(www\.)?github\.com/[\w-]+/[\w-]+`),
+	IssueTrackerGitlab:  regexp.MustCompile(`^(https?://)?[a-zA-Z0-9\-]+(\.[a-zA-Z0-9]+)+(:[0-9]+)?/[\w-]+/[\w-]+$`),
+	IssueTrackerPivotal: regexp.MustCompile(`^(https?://)?(www\.)?pivotaltracker\.com/n/projects/[0-9]+`),
+	IssueTrackerRedmine: regexp.MustCompile(`^(https?://)?[a-zA-Z0-9\-]+(\.[a-zA-Z0-9]+)+(:[0-9]+)?$`),
+}
+
 // Local todocheck configuration struct definition
 type Local struct {
 	Origin       string       `yaml:"origin"`
@@ -145,6 +153,13 @@ func (l *Local) Validate() []error {
 		errors = append(errors, err)
 	}
 
+	if l.IssueTracker != "" {
+		pattern, ok := originPatterns[l.IssueTracker]
+		if !ok || !pattern.MatchString(l.Origin) {
+			errors = append(errors, fmt.Errorf("%s is not a valid origin for issue tracker %s", l.Origin, l.IssueTracker))
+		}
+	}
+
 	return errors
 }
 


Test patch:
diff --git a/testing/test_configs/invalid_github_https.yaml b/testing/test_configs/invalid_github_https.yaml
new file mode 100644
index 0000000..b45faaf
--- /dev/null
+++ b/testing/test_configs/invalid_github_https.yaml
@@ -0,0 +1,2 @@
+origin: ws://github.com/user/project
+issue_tracker: GITHUB
diff --git a/testing/test_configs/invalid_github_origin.yaml b/testing/test_configs/invalid_github_origin.yaml
new file mode 100644
index 0000000..6f19a22
--- /dev/null
+++ b/testing/test_configs/invalid_github_origin.yaml
@@ -0,0 +1,2 @@
+origin: not-github.com/user/project
+issue_tracker: GITHUB
diff --git a/testing/test_configs/invalid_github_www.yaml b/testing/test_configs/invalid_github_www.yaml
new file mode 100644
index 0000000..badbe1b
--- /dev/null
+++ b/testing/test_configs/invalid_github_www.yaml
@@ -0,0 +1,2 @@
+origin: eee.github.com/user/project
+issue_tracker: GITHUB
diff --git a/testing/test_configs/invalid_gitlab_origin.yaml b/testing/test_configs/invalid_gitlab_origin.yaml
new file mode 100644
index 0000000..eb8e21c
--- /dev/null
+++ b/testing/test_configs/invalid_gitlab_origin.yaml
@@ -0,0 +1,2 @@
+origin: not_a_domain
+issue_tracker: GITLAB
diff --git a/testing/test_configs/invalid_gitlab_port.yaml b/testing/test_configs/invalid_gitlab_port.yaml
new file mode 100644
index 0000000..23b23ad
--- /dev/null
+++ b/testing/test_configs/invalid_gitlab_port.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com:not_a_port/user/project
+issue_tracker: GITLAB
diff --git a/testing/test_configs/invalid_jira_origin.yaml b/testing/test_configs/invalid_jira_origin.yaml
new file mode 100644
index 0000000..02e7ae2
--- /dev/null
+++ b/testing/test_configs/invalid_jira_origin.yaml
@@ -0,0 +1,2 @@
+origin: not_a_domain
+issue_tracker: JIRA
diff --git a/testing/test_configs/invalid_jira_port.yaml b/testing/test_configs/invalid_jira_port.yaml
new file mode 100644
index 0000000..09e8e01
--- /dev/null
+++ b/testing/test_configs/invalid_jira_port.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com:not_a_port
+issue_tracker: JIRA
diff --git a/testing/test_configs/invalid_pivotal_origin.yaml b/testing/test_configs/invalid_pivotal_origin.yaml
new file mode 100644
index 0000000..4ef9ea9
--- /dev/null
+++ b/testing/test_configs/invalid_pivotal_origin.yaml
@@ -0,0 +1,2 @@
+origin: https://not-pivotal.com/user/project
+issue_tracker: PIVOTAL_TRACKER
diff --git a/testing/test_configs/invalid_redmine_origin.yaml b/testing/test_configs/invalid_redmine_origin.yaml
new file mode 100644
index 0000000..ea34fe6
--- /dev/null
+++ b/testing/test_configs/invalid_redmine_origin.yaml
@@ -0,0 +1,2 @@
+origin: not_a_domain
+issue_tracker: REDMINE
diff --git a/testing/test_configs/invalid_redmine_port.yaml b/testing/test_configs/invalid_redmine_port.yaml
new file mode 100644
index 0000000..a7d3e9d
--- /dev/null
+++ b/testing/test_configs/invalid_redmine_port.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com:not_a_port
+issue_tracker: REDMINE
diff --git a/testing/test_configs/valid_github_https.yaml b/testing/test_configs/valid_github_https.yaml
new file mode 100644
index 0000000..41c6772
--- /dev/null
+++ b/testing/test_configs/valid_github_https.yaml
@@ -0,0 +1,2 @@
+origin: https://github.com/user/project
+issue_tracker: GITHUB
diff --git a/testing/test_configs/valid_github_origin.yaml b/testing/test_configs/valid_github_origin.yaml
new file mode 100644
index 0000000..b9e1eb7
--- /dev/null
+++ b/testing/test_configs/valid_github_origin.yaml
@@ -0,0 +1,2 @@
+origin: github.com/user/project
+issue_tracker: GITHUB
diff --git a/testing/test_configs/valid_github_www.yaml b/testing/test_configs/valid_github_www.yaml
new file mode 100644
index 0000000..95b4c22
--- /dev/null
+++ b/testing/test_configs/valid_github_www.yaml
@@ -0,0 +1,2 @@
+origin: www.github.com/user/project
+issue_tracker: GITHUB
diff --git a/testing/test_configs/valid_gitlab_origin.yaml b/testing/test_configs/valid_gitlab_origin.yaml
new file mode 100644
index 0000000..c329560
--- /dev/null
+++ b/testing/test_configs/valid_gitlab_origin.yaml
@@ -0,0 +1,2 @@
+origin: https://gitlab.com/user/project
+issue_tracker: GITLAB
diff --git a/testing/test_configs/valid_gitlab_port.yaml b/testing/test_configs/valid_gitlab_port.yaml
new file mode 100644
index 0000000..f032f1e
--- /dev/null
+++ b/testing/test_configs/valid_gitlab_port.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com:1337/user/project
+issue_tracker: GITLAB
diff --git a/testing/test_configs/valid_gitlab_subdomain.yaml b/testing/test_configs/valid_gitlab_subdomain.yaml
new file mode 100644
index 0000000..5709f13
--- /dev/null
+++ b/testing/test_configs/valid_gitlab_subdomain.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com/user/project
+issue_tracker: GITLAB
diff --git a/testing/test_configs/valid_jira_origin.yaml b/testing/test_configs/valid_jira_origin.yaml
new file mode 100644
index 0000000..25bf9b7
--- /dev/null
+++ b/testing/test_configs/valid_jira_origin.yaml
@@ -0,0 +1,2 @@
+origin: https://any-domain.com
+issue_tracker: JIRA
diff --git a/testing/test_configs/valid_jira_port.yaml b/testing/test_configs/valid_jira_port.yaml
new file mode 100644
index 0000000..7778a23
--- /dev/null
+++ b/testing/test_configs/valid_jira_port.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com:1337
+issue_tracker: JIRA
diff --git a/testing/test_configs/valid_jira_subdomain.yaml b/testing/test_configs/valid_jira_subdomain.yaml
new file mode 100644
index 0000000..3abcef7
--- /dev/null
+++ b/testing/test_configs/valid_jira_subdomain.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com
+issue_tracker: JIRA
diff --git a/testing/test_configs/valid_pivotal_origin.yaml b/testing/test_configs/valid_pivotal_origin.yaml
new file mode 100644
index 0000000..2714df7
--- /dev/null
+++ b/testing/test_configs/valid_pivotal_origin.yaml
@@ -0,0 +1,2 @@
+origin: https://pivotaltracker.com/n/projects/1
+issue_tracker: PIVOTAL_TRACKER
diff --git a/testing/test_configs/valid_redmine_origin.yaml b/testing/test_configs/valid_redmine_origin.yaml
new file mode 100644
index 0000000..a8ad32e
--- /dev/null
+++ b/testing/test_configs/valid_redmine_origin.yaml
@@ -0,0 +1,2 @@
+origin: https://any-domain.com
+issue_tracker: REDMINE
diff --git a/testing/test_configs/valid_redmine_port.yaml b/testing/test_configs/valid_redmine_port.yaml
new file mode 100644
index 0000000..554a77f
--- /dev/null
+++ b/testing/test_configs/valid_redmine_port.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com:1337
+issue_tracker: REDMINE
diff --git a/testing/test_configs/valid_redmine_subdomain.yaml b/testing/test_configs/valid_redmine_subdomain.yaml
new file mode 100644
index 0000000..8045ab9
--- /dev/null
+++ b/testing/test_configs/valid_redmine_subdomain.yaml
@@ -0,0 +1,2 @@
+origin: https://custom.domain.com
+issue_tracker: REDMINE
diff --git a/testing/todocheck_test.go b/testing/todocheck_test.go
index 07b3263..8d0c694 100644
--- a/testing/todocheck_test.go
+++ b/testing/todocheck_test.go
@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/preslavmihaylov/todocheck/checker/errors"
+	"github.com/preslavmihaylov/todocheck/config"
 	"github.com/preslavmihaylov/todocheck/testing/scenariobuilder"
 	"github.com/preslavmihaylov/todocheck/testing/scenariobuilder/issuetracker"
 )
@@ -330,6 +331,65 @@ func TestInvalidIssueTracker(t *testing.T) {
 	}
 }
 
+func TestInvalidOrigins(t *testing.T) {
+	invalidConfigPaths := []string{
+		"./test_configs/invalid_github_https.yaml",
+		"./test_configs/invalid_github_origin.yaml",
+		"./test_configs/invalid_github_www.yaml",
+		"./test_configs/invalid_gitlab_origin.yaml",
+		"./test_configs/invalid_gitlab_port.yaml",
+		"./test_configs/invalid_issue_tracker.yaml",
+		"./test_configs/invalid_jira_origin.yaml",
+		"./test_configs/invalid_jira_port.yaml",
+		"./test_configs/invalid_offline_url.yaml",
+		"./test_configs/invalid_pivotal_origin.yaml",
+		"./test_configs/invalid_redmine_origin.yaml",
+		"./test_configs/invalid_redmine_port.yaml",
+	}
+
+	for _, path := range invalidConfigPaths {
+		cfg, err := config.NewLocal(path, ".")
+		if err != nil {
+			t.Errorf("%s", err)
+			continue
+		}
+		errors := cfg.Validate()
+		if 0 == len(errors) {
+			t.Errorf("%s should be invalid", path)
+		}
+	}
+}
+
+func TestValidOrigins(t *testing.T) {
+	validConfigPaths := []string{
+		"./test_configs/valid_github_https.yaml",
+		"./test_configs/valid_github_origin.yaml",
+		"./test_configs/valid_github_www.yaml",
+		"./test_configs/valid_gitlab_origin.yaml",
+		"./test_configs/valid_gitlab_port.yaml",
+		"./test_configs/valid_gitlab_subdomain.yaml",
+		"./test_configs/valid_jira_origin.yaml",
+		"./test_configs/valid_jira_port.yaml",
+		"./test_configs/valid_jira_subdomain.yaml",
+		"./test_configs/valid_pivotal_origin.yaml",
+		"./test_configs/valid_redmine_origin.yaml",
+		"./test_configs/valid_redmine_port.yaml",
+		"./test_configs/valid_redmine_subdomain.yaml",
+	}
+
+	for _, path := range validConfigPaths {
+		cfg, err := config.NewLocal(path, ".")
+		if err != nil {
+			t.Errorf("%s", err)
+			continue
+		}
+		errors := cfg.Validate()
+		if len(errors) > 0 {
+			t.Errorf("%s should be valid but has errors: %v", path, errors)
+		}
+	}
+}
+
 func TestInvalidOfflineURL(t *testing.T) {
 	err := scenariobuilder.NewScenario().
 		WithBinary("../todocheck").