Summary

Start carving up RB Gateway into namespaces

Review Request #9767 — Created March 12, 2018 and submitted March 16, 2018, 12:55 p.m.

Information

Owner

brennie

Repository

rb-gateway

Branch

master

Bugs

Depends On

Blocks

9771

Commit

153760f...

Reviewers

Groups

rb-gateway

People

Description

RB Gateway currently only has a single namespace that is getting very

polluted. Because of this, methods that should be private aren't, since

they are accessible within the declared namespace (package main).
This is the first of a series of patches to split this monolithic

package into sub-packages. This patch tackles the repositories

(née repository.go and git_repository.go) package, as well as adds a

helpers package which contains all the test setup code that is used

across package boundaries.
Ideally, this would only have involved the testing infrastructure

changes, but because of how go imports work, we cannot import

github.com/reviewboard/rb-gateway from .../rb-gateway/helpers to

access Repository et al. because rb-gateway is a binary and not a

library.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
Replace with map literals.	brennie	March 13, 2018, 3:41 p.m.
This is very strange. Shouldn't we just be importing "helpers"? Even better, we should probably put all of these sub-packages …	david	March 13, 2018, 5:23 p.m.
Looks like it would be nice to expose the createTestConfig method that you currently have in util_test.go.	david	March 13, 2018, 6:21 p.m.
I think it would be a lot cleaner to have the test functions call CreateTestRepo themselves and then pass in …	david	March 13, 2018, 5:54 p.m.

flake8 passed.

JSHint passed.

helpers/repo_helpers.go (Diff revision 1)

The issue has been resolved. Show all issues

Replace with map literals.

Change Summary:

Fix test cleanups

Commit:

196fe808b5f16a41ecf96c2e08acf8e9967a6376

ed67d925b18f9ffcb7ecef2f36d07828bfa49ece

Diff:

Revision 2 (+394 -218)

Show changes

	Gopkg.lock
	Gopkg.toml
	main_test.go
	routes_test.go
	util.go
	util_test.go
	helpers/repo_helpers.go
	git_repository.go
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

routes_test.go (Diff revision 2)

The issue has been dropped. Show all issues

This is very strange. Shouldn't we just be importing "helpers"?

Even better, we should probably put all of these sub-packages into an internal directory.

brennie March 13, 2018, 5:23 p.m.

No, relative imports (it would have to be "./helpers") are not considered good practice. Also, I don't see a benefit to having this as "internal", becuase if people want they can import our packages.

david March 14, 2018, 1:49 p.m.

I don't think I want people importing our packages right now. We've made no guarantees or commitments about API stability.

routes_test.go (Diff revision 2)

The issue has been dropped. Show all issues

Looks like it would be nice to expose the createTestConfig method that you currently have in util_test.go.

brennie March 13, 2018, 6:21 p.m.

I go through and fix up these tests in a future patch. This was more about replacing checkFatal with assert.Nil etc, I just happened to clean this up a bit.

util_test.go (Diff revision 2)

The issue has been dropped. Show all issues

I think it would be a lot cleaner to have the test functions call CreateTestRepo themselves and then pass in repo.Path as a string to this method.

brennie March 13, 2018, 5:54 p.m.
```
I've fixed this in a subsequent patch.
```

Change Summary:

Fix up repo tests.

Commit:

ed67d925b18f9ffcb7ecef2f36d07828bfa49ece

153760f67fce5a04ee202d26007e9f365ab40d32

Diff:

Revision 3 (+627 -467)

Show changes

	Gopkg.lock
	Gopkg.toml
	README.md
	git_repository_test.go
	main_test.go
	routes_test.go
	util.go
	util_test.go
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (599a8b5)