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

add support aliyun dns #252

Merged
merged 3 commits into from
Oct 14, 2021
Merged

add support aliyun dns #252

merged 3 commits into from
Oct 14, 2021

Conversation

cnjack
Copy link
Contributor

@cnjack cnjack commented Oct 8, 2021

No description provided.

@cnjack
Copy link
Contributor Author

cnjack commented Oct 8, 2021

can you help me review my code? @qdm12

Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Just a few minor comments to fix up and I'll merge it.

Have you had the change to test run it? Let me know if you need help building the Docker image.

EDIT: I'm away until Tuesday so I might be not answering until then!

internal/settings/providers/aliyun/provider.go Outdated Show resolved Hide resolved
internal/settings/providers/aliyun/provider.go Outdated Show resolved Hide resolved
internal/settings/providers/aliyun/provider.go Outdated Show resolved Hide resolved
request.RR = p.host
request.RecordId = recordID

_, err = client.UpdateDomainRecord(request)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this create the domain record if it doesn't exist? Also what's the first element returned, the response? It might be worth checking what's in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think user need create the domain by themself, we needn't do create action. if recordID not found, it will return errors.ErrRecordNotFound

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. use DescribeDomainRecords check if this domain existed under this clients and get the recordID.
  2. use recordID update the domain's IP

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I know, it's just there is #129 that I'm working on for all providers.

It's fine, I'll take care of it at a later time if it's not trivial to implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, I will follow your action. let me handle it for this provider.

internal/settings/providers/aliyun/provider.go Outdated Show resolved Hide resolved
@cnjack
Copy link
Contributor Author

cnjack commented Oct 9, 2021

thanks for your review, I have pushed image in my personal account, and test it in my NAS

@cnjack cnjack requested a review from qdm12 October 12, 2021 01:08
@cnjack
Copy link
Contributor Author

cnjack commented Oct 14, 2021

hi @qdm12 , have you came back? can you help me review my code?

docs/aliyun.md Show resolved Hide resolved
@qdm12
Copy link
Owner

qdm12 commented Oct 14, 2021

Thanks @cnjack for the fixes! Just a tiny suggestion to commit in the docs/aliyun.md and I'll merge it!

@cnjack cnjack requested a review from qdm12 October 14, 2021 13:17
@cnjack
Copy link
Contributor Author

cnjack commented Oct 14, 2021

Thanks @cnjack for the fixes! Just a tiny suggestion to commit in the docs/aliyun.md and I'll merge it!

updated the document, thanks for your job

@qdm12 qdm12 merged commit 7acc9b9 into qdm12:master Oct 14, 2021
@qdm12
Copy link
Owner

qdm12 commented Oct 15, 2021

Hey @cnjack

I reworked the Aliyun code in 8a14828 to have no SDK dependency and only do http calls to their API directly. The Docker image is available qmcgaw/ddns-updater:aliyun-nosdk, could you try it sometime? Thanks!

@cnjack
Copy link
Contributor Author

cnjack commented Oct 15, 2021

No problem, I will try it.

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