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

Simplify Services.worldwide_api method #2498

Merged
merged 1 commit into from May 6, 2016

Conversation

floehopper
Copy link
Contributor

There is already a mechanism available for overriding the URI for specific services, so there is no need to have some special code to do this. Service URI's are automatically looked up via Plek and they can be overridden using environment variables of the form: PLEK_SERVICE_<service-name>_URI (see Plek documentation).

In this case, we can set the PLEK_SERVICE_WHITEHALL_ADMIN_URI environment variable to "https://www.gov.uk" if we want to use the production website to supply the data for the Worldwide API.

Although I've updated the developing-without-vm documentation, I haven't yet updated the developing-using-vm documentation, because I don't use the VM myself and so am not sure how the documentation should be changed. I have, however, confirmed that it is possible to use a locally-running version of the whitehall app to serve the Worldwide API data.

I have also updated the common-errors documentation to reflect the error that occurs if the Worldwide API is not pointing at an appropriate URI.

This PR should not be merged until I've updated the developing-using-vm documentation. Also developers should be warned about this change so they can make the appropriate changes to their local environments.

@floehopper
Copy link
Contributor Author

I'm going to remove the "Don't merge" and "Spike" labels to make it clearer that I'm looking for feedback on this PR. In any case, it wouldn't be the end of the world if it got merged as it stands.

@floehopper
Copy link
Contributor Author

Rebasing against master and force-pushing (since no commit comments).

@floehopper floehopper force-pushed the simplify-services-content-api-method branch from df394bd to 69c093e Compare May 2, 2016 11:26
@floehopper
Copy link
Contributor Author

@chrisroos, @leenagupte, @pmanrubia & @ikennaokpala:

I'd really appreciate your feedback on this PR since it would probably affect all of you and I don't know how you've got your development environments set-up.

Do any of you know about using the development VM? If so, I'd value your suggestions for how I should update the developing-using-vm documentation. Since I doubt you'll want to run a local version of whitehall, I imagine the most important thing is how to set the PLEK_SERVICE_WHITEHALL_ADMIN_URI environment variable.

@floehopper floehopper changed the title WIP: Simplify Services.worldwide_api method Simplify Services.worldwide_api method May 2, 2016
@ikennaokpala
Copy link
Contributor

ikennaokpala commented May 2, 2016

@floehopper I believe this might present an issue for non dev vm users..

Just a thought: what if we set this up with the option of an environment variable, this would help none dev vm users.

I already use the dev vm and believe this may not present any issues, will give it a go in the morning.

@floehopper
Copy link
Contributor Author

@ikennaokpala:

Thanks for your comment. However, I'm a bit confused by it. I don't use the developer VM and I have no problem running the app with the extra environment variable set. Can you elaborate on why this might be a problem?

It'd also be great to hear how you get on when you try this out on the developer VM. Thanks.

@pmanrubia
Copy link
Contributor

pmanrubia commented May 3, 2016

@floehopper: the PR looks good to me for a non-VM environment. I have had a look at the Pleck service, and it seems that the environment variable would do the trick. 👍
Unfortunately, as of today, I am not a VM developer, so I can't provide more feedback.
But if it is working for @ikennaokpala or @leenagupte (they are VM users) I would merge the PR (updating accordingly the dev-VM documentation if applies)

@ikennaokpala
Copy link
Contributor

ikennaokpala commented May 3, 2016

@floehopper at the time my comment were totally subjective and I felt that
@worldwide_api ||= GdsApi::Worldwide.new(Plek.new.find('whitehall-admin')) would be nice if it were a bit more flexible.

I have since ran tests and can confirm that all the tests pass on my dev VM and without it.

@floehopper
Copy link
Contributor Author

@ikennaokpala:

I've realised I didn't do a very good job of explaining what I wanted you to check. The Worldwide API is stubbed out during tests, so I wouldn't expect the environment variable to have any effect.

What I'm interested in is your development environment - specifically (with this branch checked out) can you view a Smart Answer page which uses the Worldwide API e.g. http://smart-answers.dev.gov.uk/marriage-abroad/y and is the country drop-down fully populated?

To replicate the previous behaviour, you'll need to set the PLEK_SERVICE_WHITEHALL_ADMIN_URI environment variable to "https://www.gov.uk" for the Smart Answers app. I don't know how to do this for the developer VM. Presumably you'll need to make a change and commit the change to a git repo somewhere so that other people can make use of your change.

The alternative is to not set this environment variable, but that means the Smart Answers app will assume there is an instance of the Whitehall app running at http://whitehall-admin.dev.gov.uk/marriage-abroad/y. It's possible that this is already the case in your developer VM, but I don't know one way or the other.

I hope the above makes sense, but if not, I'll be in the office tomorrow afternoon and we can talk it through then.

@ikennaokpala
Copy link
Contributor

ikennaokpala commented May 4, 2016

@floehopper I have done as you asked.. I use the bowler gem to start up smart-answers in the developer vm

Here is the screenshot of the response I got.

screen shot 2016-05-04 at 09 47 53
screen shot 2016-05-04 at 09 52 19

@floehopper
Copy link
Contributor Author

@ikennaokpala: Thanks. So that means that you are not running an instance of Whitehall locally, which is what I expected. Looking at the Bowler documentation it sounds as if it runs the app(s) via Foreman. I don't see a Procfile in the Smart Answers repo - is there one somewhere else? If you can find it, I imagine that you could set the PLEK_SERVICE_WHITEHALL_ADMIN_URI environment variable to "https://www.gov.uk/" in the Procfile on the line which runs the Rails app.

@ikennaokpala
Copy link
Contributor

@floehopper thanks I can confirm that the gateway exception is no longer thrown and the page loads correctly with all the countries

On the dev vm I clone and setup the whitehall repo and bowled smart-answers with whitehall i.e bowl smartanswers whitehall.

There is a Pinfile in the development repo used by bowler that define the applications and their respective dependencies.

@floehopper
Copy link
Contributor Author

Thanks to feedback from @ikennaokpala, I now know that the options are probably:

  1. Add whitehall as a dependency of smart-answers in the master Pinfile, so that you can continue to use bowl smart-answers to run the app. However, this relies on having the whitehall app setup and its database populated with sensible data. I don't know whether the latter is an unrealistic requirement.
  2. Set the environment variable in the master Procfile so that Smart Answers uses the production version of Whitehall by default when run with bowl smart-answers. It doesn't look as if any other apps are doing this, so it might not be allowed.
  3. There may be a 3rd option - if there's a way to locally override the environment used to run the command in the Procfile e.g. perhaps you can specify an env var when you run bowl and have Bowler pass on the environment to the app's server process.

I'm just waiting for confirmation that option 2 works and then I'll look at submitting one or two PRs to the dev VM repo to see what people think.

@pmanrubia
Copy link
Contributor

Great work @ikennaokpala @floehopper 👍 !!

@ikennaokpala
Copy link
Contributor

ikennaokpala commented May 4, 2016

@floehopper

Given these changes, for the dev vm you can choose one of the two options below:

  1. create an .env file in the root path of the development project that contains PLEK_SERVICE_WHITEHALL_ADMIN_URI=https://www.gov.uk/ within the development project
  2. bowling smartanswers with whitehall i.e bowl smartanswers whitehall
    • NB: make sure that you have the whitehall data dump imported prior to this

@leenagupte
Copy link
Contributor

@floehopper @ikennaokpala When you set up your dev vm to be able to run the GOV.UK stack locally, you have to run a data replication job that imports production data. Included in this is whitehall data.

It should be enough to just add whitehall as a dependency of smartanswers in the Pinfile. As with all of the other smartanswer dependencies you will need to clone the repo and then run bundle install in it.

@pmanrubia
Copy link
Contributor

pmanrubia commented May 4, 2016

Thank you @leenagupte. If we get enough data for whitehall to run smart-answers I would prefer the option 1 from: #2498 (comment)

@chrisroos
Copy link
Contributor

This looks good to me, @floehopper.

I checked out this branch and successfully ran the app using both Apache + Passenger and the Rails server.

Using Apache + Passenger

I added SetEnv PLEK_SERVICE_WHITEHALL_ADMIN_URI https://www.gov.uk to my Smart Answers virtual host config.

Rails server

I set the PLEK_SERVICE_WHITEHALL_ADMIN_URI environment variable as part of the command to start the server: PLEK_SERVICE_WHITEHALL_ADMIN_URI=https://www.gov.uk rails s.

@floehopper
Copy link
Contributor Author

Thanks for the feedback, everyone. Chatting to @leenagupte yesterday, she thinks that although it's relatively easy to run Whitehall locally using the development VM, it does use a bunch of resources and slows her machine down.

So although my option 1 (adding whitehall as a dependency in the Pinfile) seems like the most "correct" approach, I'm going to add documentation for the two options that @ikennaokpala mentioned in this comment. This will explain how to run the app with and without a local instance of Whitehall.

Since there are no commit comments, I'm planning to make the changes to the development VM docs, re-write the commit and commit note, and then rebase against master, force-push and then merge.

There is already a mechanism available for overriding the URI for specific
services, so there is no need to have some special code to do this. Service
URI's are automatically looked up via `Plek` and they can be overridden using
environment variables of the form: `PLEK_SERVICE_<service-name>_URI` (see [1]).

In this case, we can set the `PLEK_SERVICE_WHITEHALL_ADMIN_URI` environment
variable to "https://www.gov.uk" if we want to use the production website to
supply the data for the Worldwide API.

I've updated both the `developing-without-vm` and `developing-using-vm`
documentation to explain how to cope with this change.

I have also updated the `common-errors` documentation to reflect the error that
occurs if the Worldwide API is not pointing at an appropriate URI.

[1]: https://github.com/alphagov/plek/blob/b1f917cd6498aea8867728dab0132d0373ca702b/README.md#for-base-urls
@floehopper floehopper force-pushed the simplify-services-content-api-method branch from 69c093e to 9c01ec5 Compare May 6, 2016 12:15
@floehopper floehopper merged commit 0b47948 into master May 6, 2016
@floehopper floehopper deleted the simplify-services-content-api-method branch May 6, 2016 12:26
floehopper added a commit that referenced this pull request May 12, 2016
This script runs in the Rails `development` environment.

Prior to #2498, being in the `development` environment meant that the app always
used the production instance of the Whitehall app as the backend for the GDS
Worldwide API.

After #2498, by default the app would try to use a local development instance of
the Whitehall app, unless the `PLEK_SERVICE_WHITEHALL_ADMIN_URI` is set.

Since the existing `*-responses-and-expected-results.yml` files were generated
with the production instance of the Whitehall app, it seems more sensible to
set the `PLEK_SERVICE_WHITEHALL_ADMIN_URI` environment variable to ensure that
this continues to be the case.
floehopper added a commit that referenced this pull request May 13, 2016
This script runs in the Rails `development` environment.

Prior to #2498, being in the `development` environment meant that the app always
used the production instance of the Whitehall app as the backend for the GDS
Worldwide API.

After #2498, by default the app would try to use a local development instance of
the Whitehall app, unless the `PLEK_SERVICE_WHITEHALL_ADMIN_URI` is set.

Since the existing `*-questions-and-responses.yml` files were generated with the
production instance of the Whitehall app, it seems more sensible to set the
`PLEK_SERVICE_WHITEHALL_ADMIN_URI` environment variable to ensure that this
continues to be the case.
floehopper added a commit that referenced this pull request May 13, 2016
This script runs in the Rails `development` environment.

Prior to #2498, being in the `development` environment meant that the app always
used the production instance of the Whitehall app as the backend for the GDS
Worldwide API.

After #2498, by default the app would try to use a local development instance of
the Whitehall app, unless the `PLEK_SERVICE_WHITEHALL_ADMIN_URI` is set.

Since the existing `*-responses-and-expected-results.yml` files were generated
with the production instance of the Whitehall app, it seems more sensible to
set the `PLEK_SERVICE_WHITEHALL_ADMIN_URI` environment variable to ensure that
this continues to be the case.
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.

None yet

5 participants