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

Support for the custom hostnames endpoint #45

Closed
wants to merge 5 commits into from

Conversation

rdubya
Copy link
Contributor

@rdubya rdubya commented Feb 12, 2019

This adds support for custom hostnames (https://api.cloudflare.com/#custom-hostname-for-a-zone-properties).

Notes:

  • This feature is only available for plans that have it enabled
    • The custom metadata is enterprise only and needs to be enabled as well
  • I'm not sure the best approach at testing this since the only account I have to test with is production. I would typically stub out the requests but wanted to make sure whatever route I go fits in with how the maintainers would like it tested.
  • I noticed that when searching for an Account by its id, a Zone object was being returned. As part of the consolidation that I did, it now returns an Account object. I can make it continue to return a Zone if that is preferred though.

We'd like to start using this soon, so let me know if/what changes should be made and how you would prefer to see it tested.

@ioquatix
Copy link
Member

I have been having a discussion with Cloudflare trying to figure out how to test this. They don't have any sandbox account. I set up a test account for travis but it only has access to free features. I asked them to unlock all features so we could test, but so far it hasn't happened. I have linked them to this PR asking them how they think it should be tested. Let's see how it goes.

@ioquatix
Copy link
Member

@rdubya Cloudflare developer relations team has contacted me and we are trying to organise a test environment with full access to all features, so do you think you could write a spec?

@rdubya
Copy link
Contributor Author

rdubya commented Feb 16, 2019

@ioquatix Sure thing!

… the structure of the code and made it so that it could be tested on a shared account a little easier
@rdubya
Copy link
Contributor Author

rdubya commented Mar 1, 2019

Hey @ioquatix, I added in tests, let me know if there are any changes you'd like to see.

@rdubya
Copy link
Contributor Author

rdubya commented Mar 7, 2019

@ioquatix wondered if you had a chance to look over my latest changes, we're hoping to get this in production in the next week or so. Thanks!

@ioquatix
Copy link
Member

ioquatix commented Mar 7, 2019

I have emailed Cloudflare several times about some kind of facility for testing and didn't hear back from them. Let me email again. IF we don't hear back we can add the feature but will need to put the specs behind some kind of allowed failure.

Can you ensure the code is specced and the specs pass? Thanks.

@ioquatix
Copy link
Member

ioquatix commented Mar 7, 2019

Just so it is clear, I did get a reply from someone who said they were looking into it, but never heard back from them... I've just followed up with them again.

@rdubya
Copy link
Contributor Author

rdubya commented Mar 7, 2019

Ah. Yeah, the specs all pass, I have a test account that I ran them against. I also added in a couple of skips in places that I know it can fail if the feature isn't available. If you can't get an environment, I can add another environment variable like I added for the zone creation/deletion tests.

@ioquatix
Copy link
Member

ioquatix commented Mar 7, 2019

Can you pleases put all the tests which fail due to lack of permissions behind a flag?

Probably the simplest option is something like: https://relishapp.com/rspec/rspec-core/v/3-8/docs/filtering/conditional-filters

I wonder if we should expose the accounts level of access via the Account class. That way the tests would choose the correct things to run based on the connected account - i.e. if the account doesn't have access skip the tests.

@rdubya
Copy link
Contributor Author

rdubya commented Mar 7, 2019

I can look into that. For some of these features, they aren't listed at the account level so I have to use the try/rescue but I think the overall feature probably is listed in there.

@ioquatix
Copy link
Member

ioquatix commented Mar 7, 2019

You could make tests you expect to fail as pending, at least if Cloudflare ever come to the party we will know what to change/fix.

@ioquatix
Copy link
Member

ioquatix commented Mar 7, 2019

It would be nice if users such as yourself could run the tests against a live system though, and have them all green.

@ioquatix
Copy link
Member

ioquatix commented Mar 7, 2019

Okay got a reply from Cloudflare, looks like they are going to unlock my test account with all features. Fingers crossed.

@rdubya
Copy link
Contributor Author

rdubya commented Mar 8, 2019

Hey @ioquatix, I checked into skipping tests if the feature isn't enabled, but it seems like the API actually works even if the feature isn't enabled so I'm not sure we need to block it. The one thing that might apply is if the user doesn't have access to the crypto tab for their zone, but I think if we're going to start checking those types of things we should probably do it as a separate task. I do have skips in for the couple that actually throw exceptions if you don't have the feature enabled. Those tests are the ones that lead me to: socketry/async-rspec#7

I can look at it more on Monday if you still want me to add something but at this point I don't think its really needed.

@ioquatix
Copy link
Member

ioquatix commented Mar 9, 2019

I’ll look at merging this next week.

@rdubya
Copy link
Contributor Author

rdubya commented Mar 13, 2019

@ioquatix I have an additional branch ready to go that adds support for the workers KV storage endpoint, it builds on this PR so didn't know if I should wait until after this is merged to create the PR or if it was ok to merge it into this one. You can see the diff here: finalsite/cloudflare@custom_hostnames_endpoint...finalsite:workers_kv_storage_endpoint

@ioquatix
Copy link
Member

I've pulled your branch locally into the repo, and I have given you write access to the repo. Can you please do further work on that branch? That will allow CI to run correctly.

@ioquatix
Copy link
Member

Merged.

@ioquatix ioquatix closed this Mar 20, 2019
@rdubya rdubya deleted the custom_hostnames_endpoint branch April 5, 2019 17:00
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