Skip to content
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

Prepare the project to be deployed #85

Merged
merged 13 commits into from Feb 16, 2018
Merged

Conversation

dpordomingo
Copy link
Contributor

@dpordomingo dpordomingo commented Feb 12, 2018

Depends on #36 Single Binnary
Depends on #91 Remove PR from Drone config
Depends on #90 Define a persistentVolume for internal database
Depends on https://github.com/src-d/issues-infrastructure/issues/129 Push an example DB into the code-annotation volume
Depends on https://github.com/src-d/issues-infrastructure/issues/131 Push the final DB into the code-annotation volume

Many different fixes and other additions; see commit messages 🗡️

name: {{ template "fullname" . }}
spec:
accessModes:
- ReadWriteMany
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not supported by GCE

@@ -16,6 +16,7 @@ service:
externalPort: 8080
internalPort: 8080
name: code-annotation
internalDatabasePath: /var/code-annotation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not belong to the service

Copy link

@rporres rporres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the application know that database path is in /var/code-annotation?

@dpordomingo
Copy link
Contributor Author

PR description updated.
@rporres your suggestions were addressed by the separated #90 Define a persistentVolume for internal database

@dpordomingo dpordomingo changed the title [WIP][RFC] Deploy Deploy Feb 13, 2018
@dpordomingo dpordomingo changed the title Deploy Prepare the project to be deployed Feb 13, 2018
@dpordomingo dpordomingo requested review from bzz, rporres and smacker and removed request for rporres February 13, 2018 14:23
@bzz
Copy link
Contributor

bzz commented Feb 14, 2018

Updated PR description as src-d/issues-infrastructure#129 is done.

So, right now what's left before merging this seems to be:

  • fix the authentication problem (creating the OAuth app under srcd)
  • Single binary #36 to be merged
    • rebase over new master
    • sanity check
  • 8d1b6a1 to be removed

Please, feel free to update this comment in case something else is missing, etc.

@bzz
Copy link
Contributor

bzz commented Feb 14, 2018

\cc @smacker for review

Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

impossible to run locally with defaults

@@ -16,7 +16,7 @@ type appConfig struct {
Host string `envconfig:"HOST"`
Port int `envconfig:"PORT" default:"8080"`
UIDomain string `envconfig:"UI_DOMAIN" default:"http://127.0.0.1:8080"`
DBConn string `envconfig:"DB_CONNECTION" default:"sqlite://./internal.db"`
DBConn string `envconfig:"DB_CONNECTION" default:"sqlite:///var/code-annotation/internal.db"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by default server will fail because /var/code-annotation doesn't exist.
not very user-friendly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're doing prod first that's the expected location we agreed.
We could maybe change the .env.tpl to be:

DB_CONNECTION=sqlite://internal.db

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what I mean. It's absolutely okay to have /var/code-annotation path as default, but let's keep it simple for developers. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 If anybody else wonders, feedback seems to be addressed and default for dev env was changed as requested.

Makefile Outdated

# ci variables
TRAVIS_BUILD_DIR ?= $(shell pwd)
CGO_ENABLED = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at some point, it was needed, but I also needed to change a big amount of things to make the project deployable.... and maybe another change made not necessary the CGO.
Removed

Makefile Outdated
# Set enviroment variables from .env file
ENV ?= .env
$(ENV):
touch $(ENV)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer import .env only if it exists instead of creating new one. But up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 🗡️

package.json Outdated
@@ -15,7 +17,7 @@
"react-split-pane": "^0.1.74",
"redux": "^3.7.2",
"redux-devtools-extension": "^2.13.2",
"redux-little-router": "^14.2.3",
"redux-little-router": "^15.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you describe what was the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While trying to make the project buildable it was needed at some point (it was somehow related to FormidableLabs/redux-little-router#266), but it is not longer needed (idk why), so rollbacked.

Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job! don't forget to squash commit later please.

@bzz
Copy link
Contributor

bzz commented Feb 15, 2018

#36 is merged now, rebase is in order.

LGTM after that! 🎉

@dpordomingo
Copy link
Contributor Author

rebased

@dpordomingo dpordomingo merged commit 208d503 into src-d:master Feb 16, 2018
@dpordomingo dpordomingo deleted the deploy branch February 16, 2018 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants