Replaces .env file with env vars in README #170

carlosms · 2018-02-26T17:26:51Z

Fixes #120

All the env vars are explained in a table. The .env file is still suggested for development in CONTRIBUTING.

The TODOs in the table need to be with the new vars from #145.

All the env vars are explained in a table. The .env file is still suggested for development in CONTRIBUTING Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

smacker · 2018-02-26T17:29:18Z

README.md

-In order to be able to use this application while running the tool locally, make sure you add http://127.0.0.1:8080/oauth-callback to the authorization callback URL field.

-2. Copy `.env.tpl` to `.env`.
+Variable | Required | Default value | Meaning


When I view this file the table is rendered like this:

smacker · 2018-02-26T17:30:44Z

README.md


-docker run --env-file .env --rm -p 8080:8080 srcd/code-annotation
+```bash
+$ docker run --rm -p 8080:8080 srcd/code-annotation


Here should be example how to set required env variables. At least:

CAT_OAUTH_CLIENT_ID

CAT_OAUTH_CLIENT_SECRET

are there any other?

maybe we also need to provide information how to run it with DB.
Currently, it will run the server with empty DB which isn't very usable.

Done. Regarding your second comment, I think the reordering of the steps falls into #67, to be done in a future PR.

Imo, teaching to the user how to provide a db is out of scope for this PR, and should be done by #67

smacker · 2018-02-26T17:32:26Z

README.md

 #### Set the Internal Database Connection

-Before starting the application you will need to set the `CAT_DB_CONNECTION` variable in the `.env` file. It should point to the database created with the `import` command.
+Before starting the application you will need to set the `CAT_DB_CONNECTION` environment variable. It should point to the database created with the `import` command.


is default variable ok? If not it should be in docker run too.

I guess the default is OK... otherwise it should not be the default value.

I believe it's not. Opening /var/code-annotation/internal.db will fail with error: directory doesn't exist.

We have 3 options:

change defaults

change documentation

add mkdir in Dockerfile (I prefer this one)

@carlosms please address it somehow before merging (maybe just mention in another issue)

I created issue #173 so we don't loose track of this.

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

smacker · 2018-02-26T17:38:37Z

CONTRIBUTING.md


-> Please note: you will need a .env file configured with working GitHub OAuth credentials to run the application in development mode.
-> Please follow the [README Installation section](./README.md#installation) for instructions on how to do it.
+For convenience, you can configure a file with all the environment variables described in the [README](./README.md). To do so, copy `.env.tpl` to `.env`.


Sorry, for me personally this sentence isn't clear.

Maybe we should rephrase like:

For convenience, you can configure .env file with all the environment variables described in the README.
We provide a template with the configuration suitable for development in .env.tpl. You can copy it to .env and set up GitHub OAuth credentials there.

The text isn't perfect, just to give you better understanding what I mean. What do you think?

smacker · 2018-02-26T17:40:23Z

README.md

+| `CAT_ENV` | | `production` | Sets the log level. Use `dev` to enable debug log messages |
+| `CAT_HOST` | | `0.0.0.0` | IP address to bind the HTTP server |
+| `CAT_PORT` | | `8080` | Port address to bind the HTTP server |
+| `CAT_SERVER_URL` | | `<CAT_HOST>:<CAT_PORT>` | TODO |


todo? should we write a description here?

Yes, let's add the description now.
Can you please tell me the meaning of SERVER_URL, STATIC_PROXY_TARGET after the changes made in #145 ?

SERVER_URL doesn't change the meaning. It's server url available from outside. Frontend is using it to make http calls.

STATIC_PROXY_TARGET not sure we should document it. Normally nobody should change it.
And if they do it should be somehow be reflected in create-react-app. It should set HOST & PORT variables. https://github.com/facebook/create-react-app/blob/next/packages/react-scripts/scripts/start.js#L56

#145 isn't merged yet, it's one of the issues we need to resolve there. Maybe we will remove this variable and replace with something else.

smacker · 2018-02-26T17:45:08Z

README.md

 ## Installation

-## Github OAuth tokens
+## Environment Variables


Installation and Environment Variables have the same size of header. It looks a bit weird:

smacker · 2018-02-26T17:47:04Z

README.md

+The next sections make use of several environment variables to configure the application. In this table you will find all of them grouped as a quick reference:

-In order to be able to use this application while running the tool locally, make sure you add http://127.0.0.1:8080/oauth-callback to the authorization callback URL field.
+| Variable | Required | Default value | Meaning |


people often move not mandatory variables somewhere in the bottom of README. Currently, the table is larger than my display and we are going to have more... Just as an idea for improvement.

Since this README.md is already huge and imho it does not strictly follow our recommendations for README.md I would not care too much about the length of the table: I'd merge as soon as possible, and improve README.md when facing issue #67

I'd reorder the mandatory ones in a way it appears first, but it's up to you @carlosms

Thanks both for the feedback, I moved the mandatory vars to the top of the table.

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

dpordomingo

LGTM, but could you add some links?

dpordomingo · 2018-02-26T17:42:52Z

README.md

+| `CAT_ENV` | | `production` | Sets the log level. Use `dev` to enable debug log messages |
+| `CAT_HOST` | | `0.0.0.0` | IP address to bind the HTTP server |
+| `CAT_PORT` | | `8080` | Port address to bind the HTTP server |
+| `CAT_SERVER_URL` | | `<CAT_HOST>:<CAT_PORT>` | TODO |


TODO value could be

hostname:port where the API is served

dpordomingo · 2018-02-26T17:43:17Z

README.md

+| `CAT_HOST` | | `0.0.0.0` | IP address to bind the HTTP server |
+| `CAT_PORT` | | `8080` | Port address to bind the HTTP server |
+| `CAT_SERVER_URL` | | `<CAT_HOST>:<CAT_PORT>` | TODO |
+| `CAT_UI_DOMAIN` | | `<CAT_HOST>:<CAT_PORT>` | TODO |


TODO value could be

hostname:port where the frontend is served

dpordomingo · 2018-02-26T17:45:32Z

README.md

+| `CAT_PORT` | | `8080` | Port address to bind the HTTP server |
+| `CAT_SERVER_URL` | | `<CAT_HOST>:<CAT_PORT>` | TODO |
+| `CAT_UI_DOMAIN` | | `<CAT_HOST>:<CAT_PORT>` | TODO |
+| `CAT_DB_CONNECTION` | | `sqlite:///var/code-annotation/internal.db` | Points to the internal application database. Read below for the complete syntax |


I'd add the link:
syntax of the CAT_DB_CONNECTION

dpordomingo

LGTM, but I'd add some links

dpordomingo · 2018-02-26T18:26:15Z

README.md

+| `CAT_EXPORTS_PATH` | | `./exports` | Folder where the SQLite files will be created when requested from `http://<your-hostname>/export` |
+| `CAT_OAUTH_CLIENT_ID` | YES | - | GitHub application OAuth credentials |
+| `CAT_OAUTH_CLIENT_SECRET` | YES | - | GitHub application OAuth credentials |
+| `CAT_OAUTH_RESTRICT_ACCESS` | | - | Application access control based on GitHub groups or teams |


I'd add a link to #access-control

dpordomingo · 2018-02-26T18:26:35Z

README.md

+| `CAT_OAUTH_CLIENT_ID` | YES | - | GitHub application OAuth credentials |
+| `CAT_OAUTH_CLIENT_SECRET` | YES | - | GitHub application OAuth credentials |
+| `CAT_OAUTH_RESTRICT_ACCESS` | | - | Application access control based on GitHub groups or teams |
+| `CAT_OAUTH_RESTRICT_REQUESTER_ACCESS` | | - | User role control based on GitHub groups or teams |


I'd add a link to #access-control

dpordomingo · 2018-02-26T18:27:02Z

README.md

+| `CAT_UI_DOMAIN` | | `<CAT_HOST>:<CAT_PORT>` | TODO |
+| `CAT_DB_CONNECTION` | | `sqlite:///var/code-annotation/internal.db` | Points to the internal application database. Read below for the complete syntax |
+| `CAT_EXPORTS_PATH` | | `./exports` | Folder where the SQLite files will be created when requested from `http://<your-hostname>/export` |
+| `CAT_OAUTH_CLIENT_ID` | YES | - | GitHub application OAuth credentials |


I'd add a link to #github-oauth-tokens

dpordomingo · 2018-02-26T18:27:20Z

README.md

+| `CAT_DB_CONNECTION` | | `sqlite:///var/code-annotation/internal.db` | Points to the internal application database. Read below for the complete syntax |
+| `CAT_EXPORTS_PATH` | | `./exports` | Folder where the SQLite files will be created when requested from `http://<your-hostname>/export` |
+| `CAT_OAUTH_CLIENT_ID` | YES | - | GitHub application OAuth credentials |
+| `CAT_OAUTH_CLIENT_SECRET` | YES | - | GitHub application OAuth credentials |


I'd add a link to #github-oauth-tokens

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

bzz · 2018-02-27T14:51:58Z

CONTRIBUTING.md


-> Please note: you will need a .env file configured with working GitHub OAuth credentials to run the application in development mode.
-> Please follow the [README Installation section](./README.md#installation) for instructions on how to do it.
+For convenience, you can configure a `.env` file with all the environment variables described in the [README](./README.md). A template with a sample configuration suitable for development is provided in `.env.tpl`. You can copy it to `.env` and set up the variables there.


For convenience, you can create a ..

bzz · 2018-02-27T14:54:15Z

CONTRIBUTING.md

-> Please follow the [README Installation section](./README.md#installation) for instructions on how to do it.
+For convenience, you can configure a `.env` file with all the environment variables described in the [README](./README.md). A template with a sample configuration suitable for development is provided in `.env.tpl`. You can copy it to `.env` and set up the variables there.
+
+_Note_: you will need a `.env` file configured with working GitHub OAuth credentials to run the application in development mode. In order to be able to use this application while running the tool locally, make sure you use `http://127.0.0.1:8080/oauth-callback` as the "Authorization callback URL". Please follow the [README Installation section](./README.md#installation) for instructions on how to do it.


What do you think about changing
you will need a .env file ... to run the app in dev mode -> you will need to customize the configuration, either by setting env vars, or though an .env file .... to

to communicate more clearly the optional nature of .env file?

bzz · 2018-02-27T14:58:03Z

README.md

+### Github OAuth Tokens

-3. Retrieve the values for your application's Client ID and Client Secret from the [GitHub Developer Settings page](https://github.com/settings/developers) and add them to the end of the corresponding lines in .env.
+You need an OAuth application on GitHub. See [how to create OAuth applications on GitHub](https://developer.github.com/apps/building-oauth-apps/creating-an-oauth-app/). Make sure the "Authorization callback URL" points to `http://<your-hostname>/oauth-callback`.


It's always good to explain a bit of rationale for a need, so what do you think about re-phrasing by adding:
In order for authentication \w Github to work on your environment, you need to set up OAuth application on Github platform.

dpordomingo

LGTM, imo you can merge once you've addressed @bzz suggestions

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

dpordomingo

Many thanks Carlos

dpordomingo · 2018-02-27T18:20:50Z

Now that Carlos already addressed all the provided feedback, could you PTAL @bzz @smacker ?

bzz

Awesome 📜 !

smacker · 2018-02-28T11:36:20Z

README.md

+| `CAT_DB_CONNECTION` | | `sqlite:///var/code-annotation/internal.db` | Points to the internal application database. [Read below](#importing-and-exporting-data) for the complete syntax |
+| `CAT_EXPORTS_PATH` | | `./exports` | Folder where the SQLite files will be created when requested from `http://<your-hostname>/export` |
+| `CAT_ENV` | | `production` | Sets the log level. Use `dev` to enable debug log messages |
+| `CAT_STATIC_PROXY_TARGET` | | - | :exclamation: TODO: to be reviewed when [#145](https://github.com/src-d/code-annotation/pull/145) is closed |


please remove it. #145 may never be merged in master or this variable will be replaced by something

dpordomingo · 2018-02-28T13:51:38Z

@carlosms Let's remove CAT_STATIC_PROXY_TARGET as suggested by prev comment, squash, rebase and merge :)

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

Replaces .env file with env vars in README

ae52d40

All the env vars are explained in a table. The .env file is still suggested for development in CONTRIBUTING Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

carlosms requested review from bzz, dpordomingo and smacker February 26, 2018 17:26

smacker reviewed Feb 26, 2018

View reviewed changes

carlosms added 2 commits February 26, 2018 18:33

Fix table formatting

02df2ba

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

Fix table formatting... again

65bc4a4

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

smacker reviewed Feb 26, 2018

View reviewed changes

Implement feedback

ca456e8

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

dpordomingo approved these changes Feb 26, 2018

View reviewed changes

smacker mentioned this pull request Feb 26, 2018

Further improvements of documentation #67

Open

6 tasks

dpordomingo approved these changes Feb 26, 2018

View reviewed changes

Apply feedback: group mandatory vars, add links

94b098c

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

bzz reviewed Feb 27, 2018

View reviewed changes

dpordomingo approved these changes Feb 27, 2018

View reviewed changes

Small rewrite for .env file and github auth

7ba4ca2

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

carlosms mentioned this pull request Feb 27, 2018

Default env var for CAT_DB_CONNECTION does not work out of the box #173

Closed

dpordomingo approved these changes Feb 27, 2018

View reviewed changes

dpordomingo requested review from bzz and smacker and removed request for bzz and smacker February 27, 2018 18:21

dpordomingo requested review from bzz and smacker February 27, 2018 18:21

carlosms mentioned this pull request Feb 28, 2018

Documentation improvements #177

Merged

bzz approved these changes Feb 28, 2018

View reviewed changes

bzz removed the request for review from smacker February 28, 2018 11:30

smacker reviewed Feb 28, 2018

View reviewed changes

Remove STATIC_PROXY_TARGET, add UI_DOMAIN back

943ed36

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

carlosms merged commit d07124e into src-d:master Feb 28, 2018

Replaces .env file with env vars in README #170

Replaces .env file with env vars in README #170

Uh oh!

Conversation

carlosms commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smacker Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

smacker Feb 26, 2018 •

edited

Loading

dpordomingo Feb 26, 2018 •

edited

Loading

dpordomingo left a comment •

edited

Loading