Support for the custom hostnames endpoint

rdubya commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@ioquatix Sure thing!

rdubya commented 5 years ago

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

rdubya commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

ioquatix commented 5 years ago

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

rdubya commented 5 years ago

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: https://github.com/socketry/async-rspec/issues/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 commented 5 years ago

I’ll look at merging this next week.

rdubya commented 5 years ago

@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: https://github.com/finalsite/cloudflare/compare/custom_hostnames_endpoint...finalsite:workers_kv_storage_endpoint

ioquatix commented 5 years ago

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 commented 5 years ago

Merged.

socketry / cloudflare

Support for the custom hostnames endpoint #45