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 our provider to match dnsimple-ruby 4.5.0 #60

Merged
merged 9 commits into from Oct 19, 2018

Conversation

Projects
None yet
4 participants
@martinisoft
Member

martinisoft commented Oct 18, 2018

This changeset resolves the breakage that occured in the 4.5.0 release which should resolve dnsimple/dnsimple-ruby#169

martinisoft added some commits Oct 18, 2018

@martinisoft martinisoft self-assigned this Oct 18, 2018

@martinisoft martinisoft requested a review from onlyhavecans Oct 18, 2018

@onlyhavecans

Good one! Glad the tests help here. I have style suggestions though.
I'm going to mark approved as these are suggestions not requirement for merge

Show resolved Hide resolved libraries/provider_dnsimple_record.rb Outdated
Show resolved Hide resolved libraries/provider_dnsimple_record.rb Outdated
Show resolved Hide resolved libraries/provider_dnsimple_record.rb Outdated

onlyhavecans and others added some commits Oct 18, 2018

@@ -31,7 +31,7 @@ def load_current_resource
@current_resource.domain(@new_resource.domain)
@current_resource.type(@new_resource.type)
records = dnsimple_client.zones.all_records(dnsimple_client_account_id,
records = dnsimple_client.zones.list_zone_records(dnsimple_client_account_id,

This comment has been minimized.

@thekendalmiller

thekendalmiller Oct 19, 2018

Thanks for taking a look at this. I tried the patch and it does fix that error, but list_zone_records is paginated with 30 records per page so the logic below to detect if this in existing record often fails once someone has more than 30 records.

This comment has been minimized.

@thekendalmiller

thekendalmiller Oct 19, 2018

It should be all_zone_records instead

@@ -31,7 +31,7 @@ def load_current_resource
@current_resource.domain(@new_resource.domain)
@current_resource.type(@new_resource.type)
records = dnsimple_client.zones.all_records(dnsimple_client_account_id,
records = dnsimple_client.zones.list_zone_records(dnsimple_client_account_id,

This comment has been minimized.

@weppos

weppos Oct 19, 2018

Member
Suggested change Beta
records = dnsimple_client.zones.list_zone_records(dnsimple_client_account_id,
records = dnsimple_client.zones.all_zone_records(dnsimple_client_account_id,

The method was renamed from all_records to all_zone_records.

@@ -29,7 +29,7 @@
let(:response) { instance_double(Dnsimple::Response, data: data) }
let(:data) { instance_double(Dnsimple::Struct::Whoami, account: account) }
let(:account) { instance_double(Dnsimple::Struct::Account, id: 1) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, all_records: zone_records, create_record: zone_record) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, list_zone_records: zone_records, create_zone_record: zone_record) }

This comment has been minimized.

@weppos

weppos Oct 19, 2018

Member
Suggested change Beta
let(:zones) { instance_double(Dnsimple::Client::ZonesService, list_zone_records: zone_records, create_zone_record: zone_record) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, all_zone_records: zone_records, create_zone_record: zone_record) }
@@ -40,7 +40,7 @@ def response
filter = {}
filter = { type: @type } if @type
client.zones.records(account_id, @zone, filter: filter).data.detect do |record|
client.zones.list_zone_records(account_id, @zone, filter: filter).data.detect do |record|

This comment has been minimized.

@weppos

weppos Oct 19, 2018

Member
Suggested change Beta
client.zones.list_zone_records(account_id, @zone, filter: filter).data.detect do |record|
client.zones.all_zone_records(account_id, @zone, filter: filter).data.detect do |record|

This comment has been minimized.

@weppos

weppos Oct 19, 2018

Member

@martinisoft there is this one missing

@@ -40,7 +40,7 @@ def response
filter = {}
filter = { type: @type } if @type
client.zones.records(account_id, @zone, filter: filter).data.detect do |record|
client.zones.list_zone_records(account_id, @zone, filter: filter).data.detect do |record|

This comment has been minimized.

@weppos

weppos Oct 19, 2018

Member
Suggested change Beta
client.zones.list_zone_records(account_id, @zone, filter: filter).data.detect do |record|
client.zones.all_zone_records(account_id, @zone, filter: filter).data.detect do |record|

This comment has been minimized.

@weppos

weppos Oct 19, 2018

Member

Also this one

@@ -171,7 +171,7 @@
let(:response) { instance_double(Dnsimple::Response, data: data) }
let(:data) { instance_double(Dnsimple::Struct::Whoami, account: account) }
let(:account) { instance_double(Dnsimple::Struct::Account, id: 1) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, all_records: zone_records, update_record: zone_record) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, list_zone_records: zone_records, update_zone_record: zone_record) }

This comment has been minimized.

@weppos

weppos Oct 19, 2018

Member
Suggested change Beta
let(:zones) { instance_double(Dnsimple::Client::ZonesService, list_zone_records: zone_records, update_zone_record: zone_record) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, all_zone_records: zone_records, update_zone_record: zone_record) }
end
let(:client) { instance_double(Dnsimple::Client, identity: identity, zones: zones) }
let(:identity) { instance_double(Dnsimple::Client::Identity, whoami: response) }
let(:response) { instance_double(Dnsimple::Response, data: data) }
let(:data) { instance_double(Dnsimple::Struct::Whoami, account: account) }
let(:account) { instance_double(Dnsimple::Struct::Account, id: 1) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, all_records: zone_records) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, list_zone_records: zone_records) }

This comment has been minimized.

@weppos

weppos Oct 19, 2018

Member
Suggested change Beta
let(:zones) { instance_double(Dnsimple::Client::ZonesService, list_zone_records: zone_records) }
let(:zones) { instance_double(Dnsimple::Client::ZonesService, all_zone_records: zone_records) }
@weppos

@martinisoft the direct replacement of all_records is all_zone_records. I provided inline suggestions for easy approval.

@martinisoft martinisoft requested a review from weppos Oct 19, 2018

@martinisoft

This comment has been minimized.

Member

martinisoft commented Oct 19, 2018

@weppos I have replaced all the methods, thank you for the assist on this one. Waiting for a green CI build.

@martinisoft

This comment has been minimized.

Member

martinisoft commented Oct 19, 2018

@thekendalmiller Let us know how my latest updates work for you now. Did not know about the difference in methods so that should be corrected now. If all is well, I will cut a new patch release since this is also affecting us in production (yes, we dogfood our own cookbook)

@weppos

weppos approved these changes Oct 19, 2018

@thekendalmiller

This comment has been minimized.

thekendalmiller commented Oct 19, 2018

@martinisoft Thanks. It works for me now

@martinisoft martinisoft merged commit 257a790 into master Oct 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@martinisoft martinisoft deleted the bugfix/gem_updates branch Oct 19, 2018

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