-
Notifications
You must be signed in to change notification settings - Fork 67
docs: simplify outdated dev instructions #1466
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
Conversation
8ad34b2
to
8cfeb0c
Compare
d5a58c4
to
203af70
Compare
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.
Tested the commands
|
@@ -49,346 +49,125 @@ $ [sudo] make test pdb=1 | |||
$ [sudo] make test test=repos/delphi/delphi-epidata/integrations/acquisition | |||
``` | |||
|
|||
## Long version | |||
You can read the commands executed by the Makefile [here](../dev/local/Makefile). |
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 link doesnt work; it ends up pointing to https://cmu-delphi.github.io/dev/local/Makefile
... try https://github.com/cmu-delphi/delphi-epidata/blob/dev/dev/local/Makefile
instead
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.
Ah, i formatted the link so it would work here. Seems like we can either have it work on the site or work on Github. Which do we prefer? Personally, I look at the dev pages on Github.
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 think it should work for both if we set it to https://github.com/cmu-delphi/delphi-epidata/blob/dev/dev/local/Makefile
[the `fluview_meta` tutorial](new_endpoint_tutorial.md) for an example of how | ||
to add a new endpoint to the API. | ||
- the container will be able read and write to your local filesystem (which may | ||
be a security concern, especially if you are running the containers as root) |
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.
dont they always run as root?
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.
Here's a full example based on the `fluview` endpoint: | ||
What follows is a worked demonstration based on the `fluview` endpoint. Before | ||
starting, make sure that you have the `delphi_database_epidata`, | ||
`delphi_web_epidata`, and `delphi_redis` containers running; if you don't, see |
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 give docker command to check that here?
(sorry for late review points, i noticed these while doing the new version rollout) |
Np, merged this in bc I wanted @aysim319 to have the updated version. |
Summary:
Our dev instructions here are quite old and contain either out of date or duplicate information that is already in dev scripts. So here I removed some unnecessary and out of date details and pointed to the dev scripts instead. This way we don't have to keep updating this instructions along with those files. I did my best to keep the useful examples in the "Manual Tests" section around, such as loading test data into the database and querying the local API directly from the command line (though the diffs make it look like I deleted all that, it's actually kept around unaltered below).
Prerequisites:
dev
branchdev