Skip to content

Replaces .env file with env vars in README #170

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 28, 2018
Merged

Replaces .env file with env vars in README #170

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae52d40
Replaces .env file with env vars in README
carlosms Feb 26, 2018
02df2ba
Fix table formatting
carlosms Feb 26, 2018
65bc4a4
Fix table formatting... again
carlosms Feb 26, 2018
ca456e8
Implement feedback
carlosms Feb 26, 2018
94b098c
Apply feedback: group mandatory vars, add links
carlosms Feb 27, 2018
7ba4ca2
Small rewrite for .env file and github auth
carlosms Feb 27, 2018
943ed36
Remove STATIC_PROXY_TARGET, add UI_DOMAIN back
carlosms Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ plumbing: packp, Skip argument validations for unknown capabilities. Fixes #623

## Development

> 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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?


_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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?


### Global dependencies

Expand Down
39 changes: 29 additions & 10 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,42 @@ Source code annotation tool offers an UI to annotate source code and review thes

## Installation

## Github OAuth tokens
## Environment Variables
Copy link
Contributor

Choose a reason for hiding this comment

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

Installation and Environment Variables have the same size of header. It looks a bit weird:
screen shot 2018-02-26 at 18 44 48


1. 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/).
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 |
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dpordomingo dpordomingo Feb 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

| -- | -- | -- | -- |
| `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 |
Copy link
Contributor

Choose a reason for hiding this comment

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

todo? should we write a description here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

@smacker smacker Feb 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

TODO value could be

hostname:port where the API is served

| `CAT_UI_DOMAIN` | | `<CAT_HOST>:<CAT_PORT>` | TODO |
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO value could be

hostname:port where the frontend is served

| `CAT_DB_CONNECTION` | | `sqlite:///var/code-annotation/internal.db` | Points to the internal application database. Read below for the complete syntax |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add the link:
syntax of the CAT_DB_CONNECTION

| `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 |
Copy link
Contributor

Choose a reason for hiding this comment

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

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

| `CAT_OAUTH_CLIENT_SECRET` | YES | - | GitHub application OAuth credentials |
Copy link
Contributor

Choose a reason for hiding this comment

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

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

| `CAT_OAUTH_RESTRICT_ACCESS` | | - | Application access control based on GitHub groups or teams |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a link to #access-control

| `CAT_OAUTH_RESTRICT_REQUESTER_ACCESS` | | - | User role control based on GitHub groups or teams |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a link to #access-control

| | | | | |
| `CAT_JWT_SIGNING_KEY` | YES | - | Key used to sign JWT (JSON Web Tokens) in the server |

2. Copy `.env.tpl` to `.env`.
## 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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


Retrieve the values for your application's Client ID and Client Secret from the [GitHub Developer Settings page](https://github.com/settings/developers) and set them to the environment variables `CAT_OAUTH_CLIENT_ID` and `CAT_OAUTH_CLIENT_SECRET`.

### Docker

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

Choose a reason for hiding this comment

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

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

  • CAT_OAUTH_CLIENT_ID
  • CAT_OAUTH_CLIENT_SECRET

are there any other?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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. Regarding your second comment, I think the reordering of the steps falls into #67, to be done in a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

```

### Non-docker

Download binary from [releases](https://github.com/src-d/code-annotation/releases) for your platform.
Download the binary from [releases](https://github.com/src-d/code-annotation/releases) for your platform.

## Importing and Exporting Data

Expand Down Expand Up @@ -73,7 +92,7 @@ For a complete reference of the PostgreSQL connection string, see the [documenta

#### 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


This variable uses the same `DSN` string as the `import` command to point to a SQLite or PosgreSQL database.

Expand Down Expand Up @@ -107,13 +126,13 @@ http://<your-hostname>/export

The annotations made by the users will be stored in the **`assignments`** table.

## Access control
## Access Control

It is possible to restrict access and choose each user's role by adding their GitHub accounts to a specific [organization](https://help.github.com/articles/collaborating-with-groups-in-organizations/) or [team](https://help.github.com/articles/organizing-members-into-teams/).

This is optional, if you don't set any restrictions all users with a valid GitHub account will be able to login as a Requester. You may also set a restriction only for Requester users, and leave open access to anyone as Workers.

To do so, set the following variables in your `.env` file:
To do so, set the following environment variables:

* `CAT_OAUTH_RESTRICT_ACCESS`
* `CAT_OAUTH_RESTRICT_REQUESTER_ACCESS`
Expand Down