cogito: don't hardcode Github domain name #142
Conversation
| @@ -79,8 +79,10 @@ func TestRunPutSuccess(t *testing.T) { | |||
| })) | |||
| var out bytes.Buffer | |||
| var logOut bytes.Buffer | |||
|
|
|||
| gitHubSpyDomain, _ := url.Parse(gitHubSpy.URL) | |||
There was a problem hiding this comment.
this works fine for the tests...
| @@ -115,7 +117,7 @@ func TestRunPutSuccessIntegration(t *testing.T) { | |||
| var out bytes.Buffer | |||
| var logOut bytes.Buffer | |||
| inputDir := testhelp.MakeGitRepoFromTestdata(t, "../../cogito/testdata/one-repo/a-repo", | |||
| testhelp.HttpsRemote(gitHubCfg.Owner, gitHubCfg.Repo), gitHubCfg.SHA, | |||
| testhelp.HttpsRemote("github.com", gitHubCfg.Owner, gitHubCfg.Repo), gitHubCfg.SHA, | |||
There was a problem hiding this comment.
but it shouldn't work for integration test; see below why.
| @@ -133,7 +133,7 @@ func (putter *ProdPutter) ProcessInputDir() error { | |||
| case 1: | |||
| repoDir := filepath.Join(putter.InputDir, inputDirs.OrderedList()[0]) | |||
| putter.log.Debug("", "inputDirs", inputDirs, "repoDir", repoDir, "msgDir", msgDir) | |||
| if err := checkGitRepoDir(repoDir, source.Owner, source.Repo); err != nil { | |||
| if err := checkGitRepoDir(repoDir, putter.ghAPI, source.Owner, source.Repo); err != nil { | |||
There was a problem hiding this comment.
integration tests fail due to this input change.
putter.ghAPI is https://api.github.com in production; from this domain name is extracted: api.github.com and compared to what is configured in the .git/config, which is github.com. This doesn't match and the test fails..
| inputDir := testhelp.MakeGitRepoFromTestdata(t, "../../cogito/testdata/one-repo/a-repo", | ||
| testhelp.HttpsRemote("the-owner", "the-repo"), "dummySHA", wantGitRef) | ||
| testhelp.HttpsRemote(gitHubSpyDomain.Host, "the-owner", "the-repo"), "dummySHA", wantGitRef) |
There was a problem hiding this comment.
GOAL: avoid always creating a .git/config file with the domain github.com hardcoded. This makes all the tests passing because we had: left := []string{"github.com", owner, repo} in putter hardcoded also. This is a bit wrong because in the test we override the GitHub endpoint, so we should be able to propagate this value.
|
@marco-m can you please have a look and give me your opinion... |
| @@ -262,7 +262,7 @@ func TestPutterProcessInputDirSuccess(t *testing.T) { | |||
| } | |||
|
|
|||
| test := func(t *testing.T, tc testCase) { | |||
| putter := cogito.NewPutter("dummy-API", hclog.NewNullLogger()) | |||
| putter := cogito.NewPutter("https://github.com", hclog.NewNullLogger()) | |||
There was a problem hiding this comment.
this will go away in subsequent PR (support GH enterprise)
| left := []string{"github.com", owner, repo} | ||
| parsedUrl, err := safeUrlParse(domain) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
test missing...
| @@ -262,7 +262,11 @@ func checkGitRepoDir(dir, owner, repo string) error { | |||
| if err != nil { | |||
| return fmt.Errorf(".git/config: remote: %w", err) | |||
| } | |||
| left := []string{"github.com", owner, repo} | |||
There was a problem hiding this comment.
I do think that its extremely important to test and verify all 3 components.
Input from Concourse git resource can come from anywhere (gitlab, bitbucket, github enterprise, etc...) so just comparing with hardcoded github.com is wrong.
There was a problem hiding this comment.
Maybe this: https://github.com/google/go-github/blob/56ca0e5d559f691400370d2341542f06c0305df9/github/github.go#L350
will give us more insight into behavior of GitHub Enterprise...
There was a problem hiding this comment.
Input from Concourse git resource can come from anywhere (gitlab, bitbucket, github enterprise, etc...)
True, but I want to clarify something: cogito will work only in 2 cases:
- input comes from github.com. In this case we can validate before hitting the GH API.
- input comes from a github enterprise installation. In this case, there is NO way for us to actually know that it comes from a GH enterprise as opposed to, for example, from codeberg (https://codeberg.org/explore/repos). Said in another way, we cannot make a deny list and we cannot make an allow list...
This brings me back a bit to my feeling that we have to abandon validation before hitting the gh API.
On the other hand, I need to take some time to think it through, currently I have only a feeling and I might be wrong for sure. Could you please ping me when I am back? Merci.
Hopefully, this is enough to bring my point to light... but sadly I think I am a bit stuck here. I will explain in the comments.