Skip to content

Prepare the project to be deployed - #85

Merged
dpordomingo merged 13 commits into
src-d:masterfrom
dpordomingo:deploy
Feb 16, 2018
Merged

Prepare the project to be deployed#85
dpordomingo merged 13 commits into
src-d:masterfrom
dpordomingo:deploy

Conversation

@dpordomingo

@dpordomingo dpordomingo commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown

Choose a reason for hiding this comment

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

not supported by GCE

Comment thread helm-charts/code-annotation/values.yaml Outdated
externalPort: 8080
internalPort: 8080
name: code-annotation
internalDatabasePath: /var/code-annotation

Copy link
Copy Markdown

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

@rporres rporres left a comment

Copy link
Copy Markdown

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
Copy Markdown
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
@dpordomingo
dpordomingo force-pushed the deploy branch 3 times, most recently from 93e52ce to 8d1b6a1 Compare February 14, 2018 05:39
@bzz

bzz commented Feb 14, 2018

Copy link
Copy Markdown
Contributor

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.

@dpordomingo
dpordomingo force-pushed the deploy branch 2 times, most recently from c1ebb3f to 10bf191 Compare February 14, 2018 17:05
@bzz

bzz commented Feb 14, 2018

Copy link
Copy Markdown
Contributor

\cc @smacker for review

@smacker smacker left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread cli/server/server.go
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread Makefile Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Copy Markdown
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

Comment thread Makefile Outdated
# Set enviroment variables from .env file
ENV ?= .env
$(ENV):
touch $(ENV)

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done 🗡️

Comment thread package.json Outdated
"redux": "^3.7.2",
"redux-devtools-extension": "^2.13.2",
"redux-little-router": "^14.2.3",
"redux-little-router": "^15.0.0",

Copy link
Copy Markdown
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?

@dpordomingo dpordomingo Feb 15, 2018

Copy link
Copy Markdown
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.

@smacker smacker left a comment

Copy link
Copy Markdown
Contributor

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

bzz commented Feb 15, 2018

Copy link
Copy Markdown
Contributor

#36 is merged now, rebase is in order.

LGTM after that! 🎉

@dpordomingo

Copy link
Copy Markdown
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.

4 participants