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

Update to handle new Register API response format #562

Merged
merged 2 commits into from
Mar 31, 2017

Conversation

samcrang
Copy link
Contributor

The response you receive from a request for the /records endpoint on a Register is changing. This will be a breaking change. This PR provides support for both the old and new response style whilst we transition.

We're looking to publish an ADR which describes the rationable for the breaking change.

We should be deploying this change soon. We'll send a second PR to remove the code to handle the old format after we've deployed.

@samcrang
Copy link
Contributor Author

I'm assuming the build is failing because I can't trigger Travis builds with access to the necessary secrets and whatnot?

@pixeltrix
Copy link
Contributor

I'm assuming the build is failing because I can't trigger Travis builds with access to the necessary secrets and whatnot?

Yes, we have our own copy of phantomjs in a private S3 bucket due to rate limiting on the bitbucket site and Travis CI doesn't decrypt the environment vars for PRs since that could be exploited to reveal the secret. I'd welcome suggestions on how to fix that without opening up the download to the whole world. 😄

@samcrang
Copy link
Contributor Author

samcrang commented Mar 31, 2017

Makes sense. I'm assuming there's a reason you don't just run rake and let the PhantomJS gem install things? This worked locally for me.

@pixeltrix
Copy link
Contributor

Makes sense. I'm assuming there's a reason you don't just run rake and let the PhantomJS gem install things? This worked locally for me.

Yes - it's all documented in ariya/phantomjs#13953. I'm trying to just rely on the phantomjs in the path in PR #563.

@pixeltrix
Copy link
Contributor

@samcrang I've merged #563 - can you try rebasing and force pushing to update the PR, thanks.

Registers have not been returning data in this format for a long time.
No need to keep the old code around.
The response you receive from a request for the `/records` endpoint is
changing. This will be a breaking change. This change provides support
for both the old and new response style whilst we transition.

The biggest change here is the addition of a list of items under the
`item` [sic] key within each record. For the Country Register this will
only contain a single item.
@samcrang samcrang force-pushed the update-to-use-new-register-api branch from 2f960cf to 84a3f40 Compare March 31, 2017 12:44
@samcrang
Copy link
Contributor Author

Build has gone green! 😄

@pixeltrix pixeltrix merged commit 024fb1d into alphagov:master Mar 31, 2017
@pixeltrix
Copy link
Contributor

@samcrang thanks! 👍

When is the new format going live?

@samcrang
Copy link
Contributor Author

Thanks!

Hopefully Monday, 3rd April. We'll issue a clean-up PR after we've done it if that helps you out?

@pixeltrix
Copy link
Contributor

Hopefully Monday, 3rd April.

Better get a deploy out then, hadn't I 😄

We'll issue a clean-up PR after we've done it if that helps you out?

That'd be great, thanks 👍

@samcrang
Copy link
Contributor Author

Ha, totally forgot you'd still have to deploy 😄. Sorry about the super-late notice on this.

@samcrang
Copy link
Contributor Author

@pixeltrix did the deploy go okay? Are we good to deploy our side on Monday?

@pixeltrix
Copy link
Contributor

Yes, we deployed

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

2 participants