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

Fix and simplify api urls #177

Merged
merged 3 commits into from
May 11, 2016
Merged

Fix and simplify api urls #177

merged 3 commits into from
May 11, 2016

Conversation

jimallman
Copy link
Member

Support the new use of domain-only base URLs in system-config files. To do this, we tweak the internal definition of some API URLs to use the older, versioned URLs. This also modifies the on-commit webhook on GitHub.

Addresses #176. These changes are running now on devtree.

IMPORTANT: Once this PR is merged to master, we'll need to update the production api config file to match the domain-only base URLs used in devapi.

@jar398
Copy link
Member

jar398 commented May 9, 2016 via email

@jimallman
Copy link
Member Author

Compare the two linked config files (highlighted lines) to see the difference in *-BASE_URL values; these should omit the slash and path following the hostname, for example

OPENTREE_API_BASE_URL=https://${OPENTREE_API_HOST}/phylesystem/v1

should become

OPENTREE_API_BASE_URL=https://${OPENTREE_API_HOST}

Probably the simplest thing I can do is to duplicate these lines in the production file, make the desired changes, and comment them out.

@jimallman
Copy link
Member Author

On second thought, the simplest thing is a (rare) pull request in deployed-systems, which should be merged only after this one.

@jimallman
Copy link
Member Author

jimallman commented May 9, 2016

what will be the failure if I forget?

Good question. Some of these failures here can be pretty subtle, but the best "tell" will be lagging changes to studies in the index (oti).

@jar398, how would you recommend running the unit tests in a way that takes into account the production config? The steps in TESTING.md seem a bit involved (installing peyotl, cloning repos, venv?) and not safe to do even on devapi. (Or am I mistaken, and I should simply run them on devapi within our usual venv?)

@jar398
Copy link
Member

jar398 commented May 10, 2016

Which TESTING.md are you talking about, germinator or phylesystem-api? Not
sure what you mean by not safe to run, as far as I know the tests are safe,
when invoked from run_tests.sh in the germinator repo. For phylesystem-api
you have to supply the right config parameter to suppress the tests that
write phylesystem, but the germinator script does this.

I suppose we could collect all the tests in the germinator repo, so that
only it would have to be checked out, but there's an obvious tension
between modularity and dependency reduction.

I don't know about the peyotl dependency; it could be unnecessary, or easy
to work around.

Anyhow I can (& probably will) run the tests when I do the update, I
usually do. So you don't need to worry about it.

On Mon, May 9, 2016 at 7:19 PM, Jim Allman [email protected] wrote:

what will be the failure if I forget?

Good question. Some of these failures here can be pretty subtle, but the
best "tell" will be lagging changes to studies in the index (oti).

@jar398 https://github.com/jar398, how would you recommend running the
unit tests in a way that takes into account the production config? The
steps in TESTING.md seem a bit involved (installing peyotl, cloning
repos, venv?) and not safe to do even on devapi. (Or am I mistaken, and
I should simply run them on devapi within our usual venv?)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#177 (comment)

@jimallman
Copy link
Member Author

Which TESTING.md are you talking about, germinator or phylesystem-api?

Sorry, I meant the one in phlesystem-api. (I added a working hyperlink in a later edit.)

Anyhow I can (& probably will) run the tests when I do the update, I usually do. So you don't need to worry about it.

Thanks! I tried to test each URL in the system as it was changed.

@jar398 jar398 merged commit 218fe63 into master May 11, 2016
@jar398 jar398 deleted the fix-and-simplify-api-urls branch June 10, 2016 23:57
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.

2 participants