Git VCS driver: Support non-default target refs #345

rwliang · 2020-07-11T23:19:30Z

Currently, only the default ref (master) is supported.

Allow target non-default target refs to be specified via configs.
Add config-driven target ref auto-detection using the HEAD branch.
Update default repo base URL pattern to link to the content revision,
accounting for potential non-default target ref usage.
Add global-level VCS configs which are merged into the repo-level configs
during startup (repo-level vals take precedence).

salemhilal · 2020-07-22T21:12:38Z

Hey, just so that you're not left hanging, we're putting eyes on this PR next.

salemhilal · 2020-07-24T19:14:50Z

I'm still looking through this PR (nice work!) and one thing I've noticed is that this doesn't seem to work for local repositories.
I used the config below, which ended up failing to index my local folder.

{
    "max-concurrent-indexers" : 2,
    "dbpath" : "data",
    "title" : "Hound",
    "health-check-uri" : "/healthz",
    "repos" : {
        "Hound" : {
            "url" : "https://www.github.com/hound-search/hound.git"
        },
        "LocalFolder" : {
            "url" : "file:///Users/salemhilal/Code/some-local-repo",
            "vcs-config": {
                "detect-ref": false,
                "ref": "main"
            }
        }
    }
}

Specifically, it looks like it's trying to still use master as the ref, and it assumes it has a remote.

2020/07/24 15:05:35 Searcher started for Hound
2020/07/24 15:05:35 Searcher started for LocalFolder
2020/07/24 15:05:35 Failed to git fetch /Users/salemhilal/go/src/github.com/hound-search/hound/data/vcs-553b942fb36f8d5521e6625aa4157f26bd2e954c, see output below
fatal: Couldn't find remote ref master
fatal: the remote end hung up unexpectedly
Continuing...

This seems to be a bug in hound currently, but it'd be nice to support non-default target refs everywhere. I would dig into it a bit more but I'm about to be out for the weekend. I'm happy to look into this further when I'm back (mid next week).

Also, apologies in advance if I'm missing something obvious here. I'm still ramping up on the details of this codebase.

rwliang · 2020-07-24T21:33:23Z

Hey @salemhilal - thanks for taking a look! 🙏.

Hmm local repo indexing seems to be working for me with this config:

{
  "max-concurrent-indexers" : 2,
  "dbpath" : "data",
  "title" : "Hound",
  "health-check-uri" : "/healthz",
  "repos" : {
    "Hound" : {
      "url" : "https://github.com/etsy/hound.git"
    },
    "LocalFolder": {
      "url" : "file:///Users/rliang/local-rep",
      "vcs-config" : {
        "detect-ref" : false,
        "ref" : "main"
      }
    }
  }
}

master shouldn't be used in the fetch due to the explicit target ref config entry. Is it possible that the app was compiled without these changes applied?

salemhilal · 2020-07-24T21:48:51Z

Hey, that’s about where I ended up before I had to leave. I thought I had the right branch of your fork checked out, but if you can’t reproduce it that sounds like something wrong on my end. I’m unfortunately out the door already, but I’ll look at it again more closely on Monday.

salemhilal · 2020-07-27T12:44:59Z

Ok! I'm home again and can confirm that this is working as documented (I had been using go install github.com/rwliang/hound/cmds/houndd rather than setting your repo as another origin and using just go install github.com/hound-search/hound/cmds/houndd) . I'm going to put some closer eyes on the PR today and get one of the other maintainers to look, but this looks good to me.

kellegous · 2020-07-27T13:18:37Z

vcs/git.go

 }

 func (g *GitDriver) Pull(dir string) (string, error) {
-	if err := run("git fetch", dir,
+	var targetRef string
+	if len(g.Ref) > 0 {


For consistency, empty string comparisons should be g.Ref == ""

Will do - thanks!

salemhilal

I have a bunch of smaller comments, but everything here looks really good.

salemhilal · 2020-07-27T12:52:08Z

config-example.json

+            "vcs-config" : {
+                "detect-ref" : false
+            }
+        },


config.json could probably use some more complete documentation generally, but just for the sake of completeness, could you add an example of using a custom ref somewhere?

Will do - thanks!

salemhilal · 2020-07-27T12:59:47Z

config/config_test.go

+	var vcsConfigVals map[string]interface{}
+	json.Unmarshal(vcsConfigBytes, &vcsConfigVals)
+	if detectRef, ok := vcsConfigVals["detect-ref"]; !ok || !detectRef.(bool) {
+		t.Error("global detectRef vcs config setting not set for repo")


should this be a t.Fatal since vcsConfigVals is used in the test below this one?

Hmm the vcsConfigVals in the next block are pulled from another repo config and are used in a separate assertion. I think we'd want to proceed to capture all the errors in one go.

salemhilal · 2020-07-27T13:18:31Z

vcs/git.go

+	}
+	if len(targetRef) == 0 {
+		targetRef = defaultRef
+	}


Would you be willing to add some tests for this logic? Having something that enforces the precedence that these rules are evaluated could be useful.

Will do - thanks!

salemhilal · 2020-07-27T13:25:07Z

vcs/git.go

@@ -99,7 +156,7 @@ func (g *GitDriver) Clone(dir, url string) (string, error) {
 		return "", err
 	}

-	return g.HeadRev(dir)
+	return g.Pull(dir)


Can you give me more details on what this additional Pull does here?

Clone initially checks out the default HEAD branch. The Pull ensures that we check out out the target ref.

got it, much appreciated.

salemhilal · 2020-07-27T14:25:35Z

As a note, all tests are currently passing.

salemhilal · 2020-08-10T20:19:13Z

Hey @rwliang, I just wanted to bump this PR because it sounds like it'll unblock some additional work. If you're able to take care of those last handful of comments, that'd be incredibly appreciated. If not, one of us will try and address them. Thanks again either way!

rwliang · 2020-08-11T15:39:17Z

Hey @salemhilal - really sorry for the delay. I should be able to address the feedback sometime in the next two days 🙏.

rwliang · 2020-08-13T00:04:56Z

Hey folks - attempted to address the feedback. Apologies for the delays and thanks again for the reviews!

salemhilal · 2020-08-17T23:34:13Z

We have a meeting tomorrow, so with any luck we'll be able to merge this PR this week. Thanks again @rwliang!

Currently, only the default ref (`master`) is supported. - Allow target non-default target refs to be specified via configs. - Add config-driven target ref auto-detection using the `HEAD` branch. - Update default repo base URL pattern to link to the content revision, accounting for potential non-default target ref usage. - Add global-level VCS configs which are merged into the repo-level configs during startup (repo-level vals take precedence).

salemhilal · 2020-08-25T20:38:36Z

Ok, we've tested this internally with a repo that's been updated to use main. Everything works as expected.

dschott68

tested internal and LGTM

Please merge and thank you ALL for the work on this

salemhilal · 2020-08-25T20:40:20Z

@rwliang Thank you so, so much for this PR. It is hugely appreciated!

rwliang · 2020-08-25T21:22:15Z

Thanks, folks! 🙏

@blobbered

* Update config-example.json * Add CONTRIBUTING.md file * Dockerfile should use github.com/hound-search This fixing the docker build issue. * make listen URL clickable in iTerm (hound-search#340) * Add Jest tests and Prettier support (hound-search#343) This commit adds support for Jest and writes a few simple tests as proof. Adding a test is as easy as adding a file that ends in `.test.js` next to the file you want to test. Any file that matches that pattern should have access to Jest functions. The whole test suite can be run with `npm run test`. I also added support for Prettier. Running `npm run format` should run Prettier on any staged changes. I thought about adding Husky to automatically run Prettier as a pre-commit hook, but it seems as though Husky requires git v2.13 which would require me upgrading Git. I didn't add support for ESLint yet since I figured that may be a slightly more involved conversation — Prettier seems to be generally uncontroverical and is generally only slightly configurable, whereas ESlint is highly configurable and behaves however we want it to. * Git VCS driver: Support non-default target refs (hound-search#345) Currently, only the default ref (`master`) is supported. - Allow target non-default target refs to be specified via configs. - Add config-driven target ref auto-detection using the `HEAD` branch. - Update default repo base URL pattern to link to the content revision, accounting for potential non-default target ref usage. - Add global-level VCS configs which are merged into the repo-level configs during startup (repo-level vals take precedence). * Exclude file path (hound-search#347) * index: add a search option to exclude files given a regexp * api: add the excludeFiles query parameter Set the index search options ExcludeFileRegexp with this parameter to allow excluding files from the search via a regexp. * js: add the excludeFiles search parameter * css: increase the label width to fit 'Exclude file path' * js: correctly check that advanced is empty * Update README with contact info for maintainers. (hound-search#348) I don't think there's a clear way to see who has push privs to this repo from the public, and even if there is, the "Get In Touch" section should probably list the maintainers. * incorporate detailing around gopath in README's quick start instructions (hound-search#352) Co-authored-by: Rebecca Lau <[email protected]> * pin alpine version to make the docker pass * Update Dockerfile * More detailed setup documents with less assumptions on users (hound-search#359) This commit updates the readme to be a little more verbose for people who don't necessarily have Go or Docker set up, but who want to contribute nonetheless. * Apply the old patch * add some figures * remove the idea code * get it untitled * remove * add .idea dev tools * remove simple config * get config back * detailing tests and simplifying the setup * Commit TLS support guide Co-authored-by: Salem <[email protected]> * make the statement terser * delete unused img * rewording * back to the old path * Update README.md * Update README.md * Update README.md * Update README.md Co-authored-by: Salem <[email protected]> * Initialize Go Modules `go mod init github.com/hound-search/hound` * Replace jteeuwen/go-bindata with go-bindata/go-bindata The former package is unmainatained since a while and the replacement also fixes this bug I encountered when trying to build the package: jteeuwen/discussions#6 * update Go version requirement in README file * Add a -version flag to Houndd (hound-search#362) This commit adds a `-version` flag to houndd. Running `houndd -version` prints the current version and exits. Co-authored-by: Kartikay Shandil <[email protected]> * Add padding between advanced mode text inputs * add margin bottom to fields that are not last * update front end bundles Co-authored-by: Blobb Ered <@blobbered> * Setup Github Actions CI (hound-search#354) * Setup Github Actions CI Replaces Travis CI with Github Actions for better integration. Jobs are run against more recent Go versions, namely 1.12 to 1.14 . . For further documentation see - https://github.com/actions/setup-go - and https://help.github.com/en/actions This adds `-race` flag to the Go tests and will run a meta linter (golangci-lint). * keep travis in until github workflows is tested * tidy go mods * remove lint command from github workflow * add back go-bindata to go modules list * up version of actions checkout to fix deprecation errors * return go bindata to the go.sum file as well Co-authored-by: Andreas Linz <[email protected]> * Add a github actions badge Thanks again @klingtnet and @blobbered for making this possible! * Update README.md (hound-search#369) Add clarification of using file:// protocol for accessing locally pulled repos * Added new folder docs with config-options description on options. (hound-search#370) * Added new folder docs with config-options description on options. * Peer review comment update updated formatting for config options documentation and added link in readme. * readme link update * link update * Minor edits from code review Co-authored-by: kayyapil <[email protected]> Co-authored-by: ehrktia <[email protected]> Co-authored-by: Salem <[email protected]> * Test windows * Run Go build and test on Ubuntu Linux and Windows * Add lint * Install dependencies in Dockerfile * Fix typo * Adding a tip for windows users (hound-search#379) * Adding a tip for windows users * Update README.md Co-authored-by: Salem <[email protected]> Co-authored-by: Salem <[email protected]> * Use tini as default entry point (hound-search#376) * Add a warning for the default branch rename (hound-search#380) * Add a warning for the default branch rename * Add a link to Github's renaming documentation. * Update README to reflect branch rename. (hound-search#384) * Update version in main.go * support github webhook * Omit ports in constructed URL (hound-search#383) * Improve accessibility (hound-search#398) * Resolve WCAG2AA.Principle3.Guideline3_1.3_1_1.H57.2 * Resolve WCAG2AA.Principle4.Guideline4_1.4_1_2.H91.InputText.Name * Resolve WCAG2AA.Principle4.Guideline4_1.4_1_2.H91.Button.Name * Improve stats contrast * Improve advanced text contrast * Improve repo title contrast * Improve contrast in advanced section * Improve line number contrast * Improve excluded files contrast * Improve "load all" button contrast * Automatically build docker image and publish to ghcr.io (hound-search#401) * Feat: Use goreleaser * Feat: Add docker build, remove goreleaser * Fix: Add image name * Feat: Use ghcr.io and build-push-action@2 * We try again * Trying yet again * Fix: Use actor and GITHUB_TOKEN * Fix: Use github.actor in image tag path * Fix: use github.repository_owner instead of github.actor When you have an organisation, github.actor != github.repository_owner Co-authored-by: Jone Marius Vignes <[email protected]> * Update default-config.json (hound-search#391) * Update default-config.json * Update default-config.json (ref: main) * Add hyperlinks to repository root (hound-search#396) * Add literal search option (hound-search#397) * Add literal search option * Pull EscapeRegExp into common.js * Add db/ to Jest ignore patterns * Test EscapeRegExp() matches its input * Test vacuous EscapeRegExp * Give repo links a target of blank (hound-search#404) Add rel="noopener noreferrer" to _blank links Co-authored-by: Joseph Price <[email protected]> Co-authored-by: dschott <[email protected]> Co-authored-by: Nicolas R <[email protected]> Co-authored-by: Igor <[email protected]> Co-authored-by: Salem <[email protected]> Co-authored-by: Richard Liang <[email protected]> Co-authored-by: Vincent Rischmann <[email protected]> Co-authored-by: Blobb Ered <[email protected]> Co-authored-by: Rebecca Lau <[email protected]> Co-authored-by: Ruixi Fan <[email protected]> Co-authored-by: Ruixi Fan <[email protected]> Co-authored-by: Andreas Linz <[email protected]> Co-authored-by: Rebecca Lau <[email protected]> Co-authored-by: Kartikay Shandil <[email protected]> Co-authored-by: Kartikay Shandil <[email protected]> Co-authored-by: Ashley Roach <[email protected]> Co-authored-by: karthick <[email protected]> Co-authored-by: kayyapil <[email protected]> Co-authored-by: ehrktia <[email protected]> Co-authored-by: David Kolossa <[email protected]> Co-authored-by: Sandro <[email protected]> Co-authored-by: Andrew Stanton <[email protected]> Co-authored-by: David Schott <[email protected]> Co-authored-by: Cor Bosman <[email protected]> Co-authored-by: Jacob Rose <[email protected]> Co-authored-by: Alexander Chiu <[email protected]> Co-authored-by: Joel Armstrong <[email protected]> Co-authored-by: Jone Marius Vignes <[email protected]> Co-authored-by: Jone Marius Vignes <[email protected]> Co-authored-by: Simon Legner <[email protected]> Co-authored-by: Bucky Schwarz <[email protected]>

rwliang force-pushed the rl-gtr branch from be3b13b to bee95cf Compare July 12, 2020 00:05

kellegous reviewed Jul 27, 2020

View reviewed changes

salemhilal reviewed Jul 27, 2020

View reviewed changes

rwliang force-pushed the rl-gtr branch from bee95cf to bacd3ad Compare August 12, 2020 23:57

rwliang force-pushed the rl-gtr branch 3 times, most recently from a36a721 to 4d71532 Compare August 13, 2020 15:27

rwliang force-pushed the rl-gtr branch from 4d71532 to fa062c0 Compare August 18, 2020 14:42

dschott68 self-requested a review August 25, 2020 20:38

salemhilal approved these changes Aug 25, 2020

View reviewed changes

dschott68 approved these changes Aug 25, 2020

View reviewed changes

salemhilal merged commit fcb4ffd into hound-search:master Aug 25, 2020

rwliang deleted the rl-gtr branch August 25, 2020 21:22

salemhilal mentioned this pull request Oct 30, 2020

Switch default branch to main #364

Closed

dims mentioned this pull request Feb 24, 2021

Changing default branch names to main kubernetes/org#2222

Open

ianfixes mentioned this pull request Apr 2, 2021

ability to specify branches #9

Open

salemhilal mentioned this pull request Apr 5, 2021

Add ability to use custom branch from vcs config for git #275

Closed

salemhilal mentioned this pull request Nov 15, 2023

fatal: not a git repository #467

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Git VCS driver: Support non-default target refs #345

Git VCS driver: Support non-default target refs #345

rwliang commented Jul 11, 2020

salemhilal commented Jul 22, 2020

salemhilal commented Jul 24, 2020

rwliang commented Jul 24, 2020

salemhilal commented Jul 24, 2020

salemhilal commented Jul 27, 2020

kellegous Jul 27, 2020

rwliang Aug 12, 2020

salemhilal left a comment

salemhilal Jul 27, 2020

rwliang Aug 12, 2020

salemhilal Jul 27, 2020

rwliang Aug 12, 2020

salemhilal Jul 27, 2020

rwliang Aug 12, 2020

salemhilal Jul 27, 2020

rwliang Aug 12, 2020

salemhilal Aug 13, 2020

salemhilal commented Jul 27, 2020

salemhilal commented Aug 10, 2020

rwliang commented Aug 11, 2020

rwliang commented Aug 13, 2020

salemhilal commented Aug 17, 2020

salemhilal commented Aug 25, 2020

dschott68 left a comment

salemhilal commented Aug 25, 2020

rwliang commented Aug 25, 2020

Git VCS driver: Support non-default target refs #345

Git VCS driver: Support non-default target refs #345

Conversation

rwliang commented Jul 11, 2020

salemhilal commented Jul 22, 2020

salemhilal commented Jul 24, 2020

rwliang commented Jul 24, 2020

salemhilal commented Jul 24, 2020

salemhilal commented Jul 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salemhilal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salemhilal commented Jul 27, 2020

salemhilal commented Aug 10, 2020

rwliang commented Aug 11, 2020

rwliang commented Aug 13, 2020

salemhilal commented Aug 17, 2020

salemhilal commented Aug 25, 2020

dschott68 left a comment

Choose a reason for hiding this comment

salemhilal commented Aug 25, 2020

rwliang commented Aug 25, 2020