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

Remove API_HOST, WHITELISTED_IPS, associated code, and documentation #1766

Merged
merged 5 commits into from Apr 16, 2018

Conversation

Projects
None yet
2 participants
@jseppi
Contributor

jseppi commented Apr 13, 2018

As described in #1747, we don't really see any benefit in using the api.data.gov API Umbrella instance to proxy requests to our own app, and it causes a few other bugs.

This PR removes the API_HOST and WHITELISTED_IPS environment variables and the associated code for proxying API requests and whitelisting only specific IPs (those of api.data.gov) to the "real" API path. Documentation around those settings and the usage of api.data.gov is also removed.

While they will be harmless, it would be good to remove the API_HOST and WHITELISTED_IPS vars from calc-env in each of our cloud.gov spaces, but this should only be done upon promotion of these code changes to each space.

closes #1692 and closes #1694

@jseppi jseppi self-assigned this Apr 13, 2018

James Seppi

@jseppi jseppi requested a review from toolness Apr 13, 2018

@jseppi jseppi changed the title from [WIP] Remove API_HOST, WHITELISTED_IPS, and associated code to Remove API_HOST, WHITELISTED_IPS, associated code, and documentation Apr 13, 2018

@toolness

This looks good to me! As I mentioned in #1747 (comment), though I don't think we should pull the trigger on this until we've verified that we can extract the same analytics from Kibana that we can from api.data.gov. If we can't, we should add logging or something before flipping the switch.

export const API_HOST = window.API_HOST;
export const API_RATES_CSV = `${API_HOST}rates/csv/`;

This comment has been minimized.

@toolness

toolness Apr 13, 2018

Contributor

So the one thing I like about keeping these routes in their own separate file is just that they're all in one place, so maintainers (hopefully) know where to look if they decide to change any routes on the back-end, rather than relying on grepping through the source code or something. Hmmmmmmmmm. What do you think of adding a separate module called backend-routes.js that has either constants or functions (if the route requires parameters) that return route strings? Or is that going overboard?

This comment has been minimized.

@jseppi

jseppi Apr 15, 2018

Contributor

yeah, I'll add them back. Good point!

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

I ended up adding the constants to api.js instead of to constants.js or another new file. Seemed like a logical place for them to me.

This comment has been minimized.

@toolness

toolness Apr 16, 2018

Contributor

Ah nice, good idea!

### `/rates/`
You can access labor rate information at `/rates/`.
```
https://api.data.gov/gsa/calc/rates/
https://calc.gsa.gov/api/rates/

This comment has been minimized.

@toolness

toolness Apr 13, 2018

Contributor

Something this is making me realize is that API requests will now go through CloudFront on production. I don't think this currently impacts us in a negative way, but if e.g. we eventually decide to use API tokens or something, we'd have to remember to have CloudFront pass any fancy headers we use on to the origin server.

toolness and others added some commits Apr 13, 2018

@jseppi

This comment has been minimized.

Contributor

jseppi commented Apr 16, 2018

Added dependency on #1773

@jseppi

This comment has been minimized.

Contributor

jseppi commented Apr 16, 2018

Verification of analytics capabilities confimed in #1773!

@jseppi jseppi merged commit a231f2e into develop Apr 16, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate 1 fixed issue
Details
codeclimate/total-coverage 92% (0.0% change)
Details

@jseppi jseppi deleted the 1747-rm-api-host branch Apr 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment