Integrate gitcollector #51

smacker · 2019-06-17T14:30:53Z

The first part of #47

refactor workdir package to make clear separation of workdir name and
workdir path
introduce new InitWithOrgs function
use envFile struct instead of a string that knows how to generate .env
file depends on the fields
update docker-compose.yml with ghcollector and new gitbase
refactor bindings in docker-compose.yml to support docker volume as
persistent storage which allows to easily prune it and avoid performance
issues related to different host filesystem
add support for base64 workdir names

TODO in sequential PRs because this PR is rather big already:

rename init into local init
add validation for token and organization names correctness
there is initial support for multi-orgs but ghcollector supports only 1 for now. Most probably it also makes sense to sort them and maybe some other changes that are better to implement with underlying support is ready
move orgs workdirs into orgs subdir and local workdirs into local subdir (maybe)

smacker · 2019-06-17T15:03:29Z

Ooops. The last change broke it. I'm fixing it.

smacker · 2019-06-17T15:25:20Z

Nobody reviewed it yet, so I just force-pushed the fix

dpordomingo

suggestions for command descriptions

cmd/sourced/cmd/orgs.go

docker-compose.yml

se7entyse7en · 2019-06-18T08:37:00Z

cmd/sourced/compose/workdir/org.go

+
+// InitWithOrgs initialize workdir with remote list of organizations
+func InitWithOrgs(orgs []string, token string) (string, error) {
+	workdir := base64.StdEncoding.EncodeToString([]byte(strings.Join(orgs, ",")))


Shouldn't we need to sort orgs first?

please take a look in the description. It's in todo for multi-org support. Currently, it works only with 1 org. There is no need to sort 1 org.

ops, sorry I missed that. 👍

se7entyse7en · 2019-06-18T09:03:08Z

cmd/sourced/compose/workdir/common.go

+		return "", err
+	}
+
+	realPath, err := filepath.EvalSymlinks(path)


Suggested change

realPath, err := filepath.EvalSymlinks(path)

realPath, err := ActivePath()

se7entyse7en · 2019-06-18T09:05:57Z

cmd/sourced/compose/workdir/common.go

+}
+
+// common initialization for both local and remote data
+func initWorkdir(workdirPath string, makeEnvContent func() string) error {


Why not envContent envFile insteaf of makeEnvContent func() string?

hm. Makes sense to change. I added envFile after I wrote this function.

- refactor workdir package to make clear separation of workdir name and workdir path - introduce new InitWithOrgs function - use envFile struct instead of a string that knows how to generate .env file depends on the fields - update docker-compose.yml with ghcollector and new gitbase - refactor bindings in docker-compose.yml to support docker volume as persistent storage which allows to easily prune it and avoid performance issues related to different host filesystem - add support for base64 workdir names Signed-off-by: Maxim Sukharev <max@smacker.ru>

Signed-off-by: Maxim Sukharev <max@smacker.ru>

smacker · 2019-06-18T11:01:07Z

@dpordomingo @se7entyse7en please another pass.

Signed-off-by: Maxim Sukharev <max@smacker.ru>

smacker · 2019-06-18T12:35:04Z

Updated to ghcollector beta.3 and updated the config for srcd-ui to match what current master expects.
Update to beta.3 doesn't mean progress works, see: src-d/gitcollector#35

dpordomingo

outdated comment; you can use message history if you want to make laughs about me ;)

smacker · 2019-06-18T14:42:28Z

I'll recheck. I didn't retest local mode after the very recent changes.

dpordomingo · 2019-06-18T14:57:40Z

You can disregard my prev comment; the problem I had with GITBASE_REPOS_DIR was caused by an old version of docker-compose.yml

dpordomingo

Tried sourced init and sourced orgs init, and they both works, so LGTM.
Reviewed code a bit fast, not in full depth because some parts were moved from A to B and diff is a bit ugly.

dpordomingo · 2019-06-18T15:08:28Z

cmd/sourced/compose/workdir/org.go

+// InitWithOrgs initialize workdir with remote list of organizations
+func InitWithOrgs(orgs []string, token string) (string, error) {
+	workdir := base64.StdEncoding.EncodeToString([]byte(strings.Join(orgs, ",")))
+	workdirPath, err := absolutePath(workdir)


We mentioned that workdirs from orgs will live in orgs dir. It can be done in another PR, but it could be added in TODO PR description to avoid forgetting it

thanks. updated

Is it possible you forgot to push? I don't see this change

TODO is updated. Not the code.

oh, you mean updated the description, I missed it on the first read

move orgs workdirs into orgs subdir and local workdirs into local subdir (maybe)

dpordomingo · 2019-06-18T15:11:50Z

cmd/sourced/compose/workdir/common.go

+	GITBASE_VOLUME_SOURCE=%s
+	GITBASE_SIVA=%s
+	GITHUB_ORGANIZATION=%s
+	GITHUB_TOKEN=%s


I think we could omit GITBASE_SIVA, GITHUB_ORGANIZATION, GITHUB_TOKEN when not in an org .env

the idea is not to use ${var:-} in docker-compose.yml so it easy to spot if something is broken (.env file and yml file aren't in sync). It complains if there is nothing set in such case.
And to avoid warning for a normal workflow we set env vars into empty values.

Though I don't feel strongly about it. We can change later if everybody prefers.

Signed-off-by: Maxim Sukharev <max@smacker.ru>

se7entyse7en · 2019-06-18T16:43:36Z

docker-compose.yml

@@ -7,17 +7,37 @@ services:
      - 9432:9432
    restart: always

+  gitcollector:
+    image: srcd/gitcollector:v0.0.1-beta.3
+    # wait for db


Maybe dumb question, but which db? The superset's postgres? Isn't the

depends_on: - postgres

enough?

no. not enough. depends_on handles only container creation. But postgres needs time to init and start before becoming available. You can read more about it here: https://docs.docker.com/compose/startup-order/

thanks for the link!

se7entyse7en

LGTM

carlosms · 2019-06-19T09:54:21Z

docker-compose.yml

+      GITHUB_ORGANIZATION: ${GITHUB_ORGANIZATION:-}
+      GITHUB_TOKEN: ${GITHUB_TOKEN:-}
+      # use main db
+      GITCOLLECTOR_METRICS_DB_URI: postgresql://superset:superset@postgres:5432/superset?sslmode=disable


Why do we need to use the main DB? Is it not better to use the metadata db?
I'm assuming at some point we may have a command to cleanup the gitcollector+ghsync data and download again. Removing the metadata db and gitbase_repositories volumes will leave everything in a clean state.

I used main db exactly to avoid wiping out stuff related to gitcollector.
I assumed it can update sivas (as it was shown during the demo). But maybe it's not true. I asked for confirmation.

it will update the discovered repositories which are already there, there's is an option to don't allow updates so it would do nothing

confirmed

I'm not too sure about this. In my mind it would be better to have gitcollector and ghsync totally in sync.
gitcollector can update sivas, but can it detect if a repo was deleted, or renamed?

Anyway to me this is something worth discussing, but for the initial download it's ok either way.

carlosms

There is a missing base64 decode somewhere, I found this bug:

$ ./sourced workdirs
  carlosms-test-org
* /home/cmartin/repos

$ ./sourced prune --all
lstat /home/cmartin/.sourced/workdirs/carlosms-test-org: no such file or directory

$ tree -a .sourced/workdirs/
.sourced/workdirs/
├── __active__ -> /home/cmartin/.sourced/workdirs/carlosms-test-org
├── home
│   └── cmartin
│       └── repos
│           ├── docker-compose.yml -> /home/cmartin/.sourced/compose-files/__active__/docker-compose.yml
│           └── .env
└── Y2FybG9zbXMtdGVzdC1vcmc=
    ├── docker-compose.yml -> /home/cmartin/.sourced/compose-files/__active__/docker-compose.yml
    └── .env

Exported ListPath and SetActivePath functions to support prune all logic. Another option would be to incapsulate this logic inside workdir package. Signed-off-by: Maxim Sukharev <max@smacker.ru>

smacker · 2019-06-19T10:54:16Z

Thanks. Fixed.

Exported ListPath and SetActivePath functions to support prune all
logic. Another option would be to encapsulate this logic inside workdir
package.

Though I would prefer to unify workdir names instead (base64 everywhere as local won't be the main use case, it's much easier to use org), it would simplify everything a lot.

Signed-off-by: Maxim Sukharev <max@smacker.ru>

smacker · 2019-06-19T11:13:51Z

Ooop. Everybody missed hard-coded SYNC_MODE: true for ui container. I hot fixed it by using GITBASE_SIVA. We can improve it later.

smacker requested a review from a team June 17, 2019 14:30

smacker mentioned this pull request Jun 17, 2019

Integrate ghsync #52

Closed

dpordomingo reviewed Jun 17, 2019

View reviewed changes

cmd/sourced/cmd/orgs.go Outdated Show resolved Hide resolved

cmd/sourced/cmd/orgs.go Outdated Show resolved Hide resolved

cmd/sourced/cmd/orgs.go Outdated Show resolved Hide resolved

smacker mentioned this pull request Jun 17, 2019

Ghsync integration #54

Merged

se7entyse7en reviewed Jun 18, 2019

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

se7entyse7en reviewed Jun 18, 2019

View reviewed changes

se7entyse7en suggested changes Jun 18, 2019

View reviewed changes

smacker added 3 commits June 18, 2019 12:55

Improve documentation for orgs subcommand

abca2df

Signed-off-by: Maxim Sukharev <max@smacker.ru>

Simplify common code for workdir package

68de151

Signed-off-by: Maxim Sukharev <max@smacker.ru>

Update gitcollector to beta.3 and ui config to match current master

9be6cb7

Signed-off-by: Maxim Sukharev <max@smacker.ru>

dpordomingo reviewed Jun 18, 2019

View reviewed changes

dpordomingo approved these changes Jun 18, 2019

View reviewed changes

update sourced-ui to v0.3.0-beta.2

ef7da0e

Signed-off-by: Maxim Sukharev <max@smacker.ru>

se7entyse7en reviewed Jun 18, 2019

View reviewed changes

se7entyse7en approved these changes Jun 18, 2019

View reviewed changes

carlosms reviewed Jun 19, 2019

View reviewed changes

carlosms suggested changes Jun 19, 2019

View reviewed changes

Fix prune --all

220f518

Exported ListPath and SetActivePath functions to support prune all logic. Another option would be to incapsulate this logic inside workdir package. Signed-off-by: Maxim Sukharev <max@smacker.ru>

carlosms approved these changes Jun 19, 2019

View reviewed changes

Fix SYNC_MODE for ui enable only for orgs mode

e05407b

Signed-off-by: Maxim Sukharev <max@smacker.ru>

smacker merged commit c2335eb into src-d:master Jun 19, 2019

carlosms mentioned this pull request Jun 19, 2019

Integrate gitcollector #47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate gitcollector #51

Integrate gitcollector #51

smacker commented Jun 17, 2019 •

edited

smacker commented Jun 17, 2019

smacker commented Jun 17, 2019

dpordomingo left a comment

se7entyse7en Jun 18, 2019

smacker Jun 18, 2019

se7entyse7en Jun 18, 2019

se7entyse7en Jun 18, 2019

se7entyse7en Jun 18, 2019

smacker Jun 18, 2019

smacker commented Jun 18, 2019

smacker commented Jun 18, 2019

dpordomingo left a comment •

edited

smacker commented Jun 18, 2019

dpordomingo commented Jun 18, 2019

dpordomingo left a comment

dpordomingo Jun 18, 2019

smacker Jun 18, 2019

carlosms Jun 19, 2019

smacker Jun 19, 2019

carlosms Jun 19, 2019

dpordomingo Jun 18, 2019

smacker Jun 18, 2019

se7entyse7en Jun 18, 2019

smacker Jun 18, 2019

se7entyse7en Jun 18, 2019

se7entyse7en left a comment

carlosms Jun 19, 2019

smacker Jun 19, 2019

smacker Jun 19, 2019

carlosms Jun 19, 2019

carlosms left a comment

smacker commented Jun 19, 2019 •

edited

smacker commented Jun 19, 2019

	realPath, err := filepath.EvalSymlinks(path)
	realPath, err := ActivePath()

Integrate gitcollector #51

Integrate gitcollector #51

Conversation

smacker commented Jun 17, 2019 • edited

smacker commented Jun 17, 2019

smacker commented Jun 17, 2019

dpordomingo 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

smacker commented Jun 18, 2019

smacker commented Jun 18, 2019

dpordomingo left a comment • edited

Choose a reason for hiding this comment

smacker commented Jun 18, 2019

dpordomingo commented Jun 18, 2019

dpordomingo 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

Choose a reason for hiding this comment

se7entyse7en 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

carlosms left a comment

Choose a reason for hiding this comment

smacker commented Jun 19, 2019 • edited

smacker commented Jun 19, 2019

smacker commented Jun 17, 2019 •

edited

dpordomingo left a comment •

edited

smacker commented Jun 19, 2019 •

edited