-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CI, docs and fix integration tests #15
Conversation
0ad81d3
to
1c89a64
Compare
idk why DCObot says that last commit 1c89a64 is not signed-off :S |
👏 for detailed description!! I assume that all commits with the message staring with WORKAROUND are not proposed to be merged, is that correct? It's totally fine to have a branch with those and even a PR with Meanwhile I assume only master...dpordomingo:478c14d needs to be reviewed |
Depending on the missing services or dependencies, they would be needed. For example the one with the The benefit on having separated commits for them, is that now we can rollback the separately to restore the expected behavior. And also that the commits being reviewed does not include «hack things» |
Let's just let CI fail, if something is missing and fix that gradually and visibly, in subsequent PRs if needed, rather then use personal forks. For me, it's ok to keep here only the fork of
Any other action items to fix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Left a few minor comments.
README.md
Outdated
|
||
Launch [bblfshd](https://github.com/bblfsh/bblfshd) and install the drivers. More info in the [bblfshd documentation](https://doc.bblf.sh/user/getting-started.html). | ||
Web application powered by [gitbase](https://github.com/src-d/gitbase), were you can query git repositories with a MySQL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this sentence a bit confusing. What about something like:
Web application to query git repositories using SQL. Powered by gitbase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great! thanks!
|
||
Built binary: | ||
Read [more about how to run bblfsh and gitbase dependencies](docs/quickstart.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion we could include the contents here in the main readme. These are mandatory dependencies, without it you can't do anything. And the quickstart doc is not so big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the same.
Our documentation guidelines suggest to consider the README.md
an "overview of the project", and suggest adding only "basic installation instructions. If the instructions are long they should be part of a different document linked from this one."
Our guides also consider the README.md
as an "index for all other documentation" so it seems that using other docs for more details is something welcomed.
And I find another reason to do so: The instalation of the dependencies is not something "core" in the project, something so relevant that should be moved to the main README.md
; on the other hand, if we move them to a separate doc, it can be considered as a minor thing more easily (edited, deleted...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining the reasoning. I still find it confusing that the readme includes "incomplete" instructions. What is GITBASE_CONTAINER
referring to a few lines below? As the guidelines say, maybe this should all be moved to one place only, the quickstart guide?
Anyway it's just a matter of opinion, up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting the GITBASE_CONTAINER
leftover (it came from the quickstart.md
).
I fixed it a87c446 to be consistent with the whole docker command: "link with a gitbase
container, and use it as a connection addres".
Many thanks for taking your time in reviewing so deeply the README.md
👍; I appreciate it a lot, and I'm sure that @campoy will do it too once this project gets more mature.
@@ -27,7 +27,7 @@ type appConfig struct { | |||
Host string `envconfig:"HOST" default:"0.0.0.0"` | |||
Port int `envconfig:"PORT" default:"8080"` | |||
ServerURL string `envconfig:"SERVER_URL"` | |||
DBConn string `envconfig:"DB_CONNECTION" default:"root@tcp(localhost:3306)/none?maxAllowedPacket=4194304"` | |||
DBConn string `envconfig:"DB_CONNECTION" default:"gitbase@tcp(localhost:3306)/none?maxAllowedPacket=4194304"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this working out of the box? do you need any extra steps to configure gitbase to accept this user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the gitbase
default https://github.com/src-d/gitbase/blob/master/Dockerfile#L18 and it worked out of the box with that username (with root
failed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I was running it from the source code, where root
is the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this is very confusing.
It would be nice to add a small notice to "Running from Source" section of contributing.md about this - that in case of running gitbase from source, this ENV bar becomes a must.
That would save some frustration for people who read the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking in purposing the refactor of the DB_CONNECTION
parameter to be:
DB_USER default:"gitbase"
DB_PASSWORD
DB_HOST default:"localhost"
DB_PORT default:"3306"
That way it would be easy to documment each one, and to modify only some parts of the connection string.
But I thought it would be better to handle this suggestion in a separated issue.
@bzz Would you agree on discussing it in the next iteration? If you do, I'll open an issue with your suggestions and my ideas.
docs/CONTRIBUTING.md
Outdated
``` | ||
|
||
|
||
## Run from sources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Titlecase: Run from Sources
docs/CONTRIBUTING.md
Outdated
|
||
It will test that `gitbase` and `bblfshd` are running and available for the playground. | ||
|
||
It is needed to serve under `gitbase` a copy of the `https://github.com/src-d/gitbase-playground` repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"serve under gitbase
" sound a bit confusing to me. This section would be much more clear if you copied the instructions from the quickstart doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quickstart has a different purpose; it's about dependencies, but this one is about the integration tests requirements.
Would you prefer @carlosms this other paragraph? https://github.com/src-d/gitbase-playground/pull/15/files#diff-8595297407c24b9ccb87b2a698cca9c2R80
To let the tests succeed, it will be also needed to configure the running
gitbase
to serve a copy of thehttps://github.com/src-d/gitbase-playground
repository.
Do you have any other suggestion ? 😉
docs/quickstart.md
Outdated
@@ -0,0 +1,70 @@ | |||
# Quickstart | |||
|
|||
## bblfsh and gitbase dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title case: Dependencies
docs/quickstart.md
Outdated
You have more information on the playground architecture and development guides in the [CONTRIBUTING.md#development](CONTRIBUTING.md#development). | ||
|
||
|
||
## run a query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title case: Run a Query
@bzz I removed the The missing ones are:
|
docs/CONTRIBUTING.md
Outdated
- serving the app for development purposes, [building and running it from sources](#run-from-sources) | ||
|
||
|
||
# Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this section should be moved to the quickstart guide? These env vars are also relevant to people interested in running the released binaries or docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not pretty sure, but I have no strong opinion at all about it because I see the benefit of both approaches.
If you agree, we can take care of the docs in a next iteration considering this, and other things like docsrv
, index with docs, running examples, hot reloading...
But if you prefer to move now the Configuration
section from the CONTRIBUTING.md
to the quickstart.md
just say so and I'll do it.
In the meantime, I added 98fc7ef#diff-018e220b50f876168d6d1d4dbf52ce84R65 in the quickstart.md
a link to this section, so as you said "people interested in running the released binaries or docker" will be benefited from the crossing links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
README.md
Outdated
|
||
Use `GITBASEPG_ENV=dev` for extra logs information. | ||
## Run the playground |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title case
README.md
Outdated
Built binary: | ||
Read [more about how to run bblfsh and gitbase dependencies](docs/quickstart.md). | ||
|
||
### Run with docker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title case
README.md
Outdated
``` | ||
|
||
### Run the Tests | ||
|
||
### Run binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found few small issues and few questions
$(GOVET) $@ | ||
|
||
back-start: | ||
GITBASEPG_ENV=dev go run cmd/gitbase-playground/main.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we agreed to be prod
by default for our applications, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but back-start
target is meant for dev purposes only.
Would you prefer it to be launched with prod
conf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's only for development I would prefer to run it with proper dev
environment.
But looks like it's not only dev command. In serve
rule you call back-start
. And make serve
is a recommended way to run our applications (at least 2 others of them) if the one doesn't want to use docker.
If I'm not mistaken in code annotation you suggested to run apps in prod mode for such case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right @smacker : I purposed (and purpose) to set the defaults to prod
settings when it is being used for final users. The rationale in behind would be
final users should be able to run the app with as less configuration effort as possible
Since this tool was purposed to be a final user app, the README.md
only recommends to run the app with docker or through a released binary
Because of that reason: "to make the tool easy to run for the final user", with as less dependencies as possible (only the really needed), the make serve
recommendation was moved to the CONTRIBUTING.md#development section.
If all of you think that we should recommend to run the tool that other way (with go
, GOPATH
, yarn
...) then I'd move it to the README.md
and remove the dev
conf in the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like using make only for dev 👍
Makefile
Outdated
|
||
# this::build -> Makefile.main::build -> Makefile.main::$(COMMANDS) | ||
build: front-build back-build | ||
@echo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please explain why do we need echo here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it is needed to run this target before the Makefile.main::build
one.
If you drop the recipe inside the target, it is run following the declaration order, what would be wrong in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wah. could you please add a comment about it. I see
this::build -> Makefile.main::build -> Makefile.main::$(COMMANDS)
comment, but I didn't get it. Your explanation is much better.
no-changes-in-commit | ||
|
||
exit: | ||
exit 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do you use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the receipes from Makefile.main::dependencies
must not be called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Could you please also add a comment. Makefiles are tricky and we don't work with them often, so I would really appreciate documentation for such non-standard cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do so.
Makefile
→ 💥
Makefile
Outdated
GOVET := go vet | ||
BINDATA := go-bindata | ||
DIFF := diff | ||
GITADD := git add -A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do you use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover; good catch
docs/CONTRIBUTING.md
Outdated
|
||
There are two main different ways to run the app: | ||
|
||
- using released artifacts, as [listed in the README.md#](../README.md#run-the-playground) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listed in the README.md#
-> listed in the README.md
docs/quickstart.md
Outdated
--publish 9432:9432 | ||
--volume /var/lib/bblfshd:/var/lib/bblfshd | ||
--name ${BBLFSHD_CONTAINER} | ||
${BBLFSHD_IMAGE}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why some commands end with ;
and others not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because you looked deeper than I did ;)
Fixed; thanks!!!
@@ -0,0 +1,40 @@ | |||
// satisfies go-bindata interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bak file in the repository looks weird...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to suggestions ;)
The point is that I think we should ensure we do not destroy this file... but how to do it...
ideas? 💃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this PR it's maybe alright. But I would like to re-think how we work with assets. Maybe we can find better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ that's a great idea. I opened an issue #24 to avoid losing your valuable suggestion.
@@ -6,4 +6,5 @@ import "database/sql" | |||
type SQLDB interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I understand we need this interface only for testing purposes.
any particular reason to mock it on this level instead of creating mocked sql.DB
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline:
- we call
.Close()
on db so just&sql.DB{}
wouldn't work - bringing any mocking dependency is out of scope for now
- though @dpordomingo is unhappy with this interface too :)
Thanks for your detailed review @smacker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, wondering if we should merge as is, or change gitbase image name right now accounting for a src-d/gitbase#262 soon to be foxed
docs/quickstart.md
Outdated
```bash | ||
$ GITBASE_CONTAINER=gitbase | ||
$ PLAYGROUND_CONTAINER=gitbasePlayground | ||
$ PLAYGROUND_IMAGE=src-d/gitbase-playground:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all 3 quickstart examples, what is the value having user to read and copypaste named ENV vars?
It's 9 extra LoC in docs and quickstart usually is shorter the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me using "variables" is more clear, because it exposes what things are in common between different steps, and the meaning in behind some names.
But if you prefer to provide only a "run this command, and go", instead of a kind of "tutorial", just confirm it, and I'll do the changes 🤝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Yes, I belive "run this command, and go"
would fit best for quickstart.
Ant thank you for being receptive and open-minded about such feedback! 💪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for suggesting things 💃
done by fcb3973
docs/quickstart.md
Outdated
$ REPOS_PATH=~/gitbase/repos | ||
$ GITBASE_CONTAINER=gitbase | ||
$ BBLFSHD_CONTAINER=bblfsh | ||
$ GITBASE_IMAGE=dpordomingo/gitbase:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed untill src-d/gitbase#262 resolved
I would put a comment right in the doc, if upstream issue does not get resolved quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do so
.travis.yml
Outdated
|
||
stages: | ||
- name: validate and build | ||
if: >- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this condition meant to be... always run? You could just omit the if
field here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm... there are some cases where this stage won't run... but who cares... we can get rid of this tricky if
, and run the tests always; I'll do so.
.travis.yml
Outdated
- docker | ||
|
||
go: | ||
- "1.9.x" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.10.x
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and tip
with allow_failures
, you're right @smola
I tried, but something was missing in the matrix... and some stages was not run when I tried it...
Could we open an issue with this requirements, merge as it is, and then proceed with it after merging?
If you agree, I'll open the issue to avoid forgeting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As agreed offline, we'll go with go 1.10.x
and we'll iterate later with 1.9.x
and tip
.
I opened an issue #25 to ensure we address this requirement, and I'll change the 1.9.x
with 1.10.x
in this PR to be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done by 10fd2d7
.travis.yml
Outdated
# push to dockerhub | ||
- stage: push to dockerhub | ||
script: | ||
- make build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid building binaries for all OSes and arches here, building only the one that we use for Docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right @smola
What if I change it with:
script:
- PKG_OS=linux make build
- DOCKER_PUSH_LATEST=true make docker-push
(I realized that even removing the make build
command, the Darwin
binary will be also generated, so it seems to be needed the PKG_OS=linux
env var, isn't it?)
We also need the make build
command because there are "other things" that needs to be built for the docker image.
This PR obtained a lot of feedback, many thanks folks!!! Here are the last changes I did to address your comments:
Addressed with comment, so I'd need your approval or not:
|
thank you @dpordomingo for such detailed followup 👏 Looks great to me. Should be ready to merge after @smacker does another pass. |
All comments addressed; I'll squash and let you know once this is ready to be merged :D |
999f97d
to
62975b5
Compare
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
- Avoid running integration tests as default ones - Rename integration tests as '*_integration_test.go' Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
Travis succeeded with |
required by #1
address #5
I ran it in my own Travis, in a private PR dpordomingo#8 and here is the result:
pull_request
→ PR review stage 1master
→ integration review stage 1tag
→ release docker images and GH assets stage 1+2+3Tag with assets to validate (darwin and linux) → dpordomingo/gitbase-playground#v0.0.1
Docker image to validate → hub.docker.com/dpordomingo/gitbase-playground
Assumptions for the first iteration
dpordomingo
accounts for DockerHub and Travis till there are company ones,dpordomingo/ci
because there are PR insrc-d/ci
that has not been merged yet,(Fix version variable ci#67 and Let the validation to check untracked files ci#62)
make
rules are copied fromcode-annotation
(it might change in the next iteration),Documentation
It was added:
README.md
docs/
CONTRIBUTING.md
quickstart.md
guideline to run the playground with its dependenciesCODE_OF_CONDUCT.md
andDCO
CI in Travis:
validate and build
to ensure the QA, that runs onpull_request
,tag
,master
andstaging
,release to github
to publish build assets (linux/darwin) to GH release on everytag
,push to dockerhub
to publish the docker image (+:latest
) to DockerHub on everytag
,src-d/ci::v1
api as possible,make validate-commit
checks that:./server/assets/asset.go
proxy was not ruined,Integration tests
They where added by #10, but as pointed by #10 (comment) they cannot be run in Travis yet.
It was also done:
*_integration_test.go
,make back-test-integration
to run them locally (it is needed an instance ofbblfsh
andgitbase
)