Skip to content

feat: Added Netlify as a provider. - #1052

Open
CompewterTutor wants to merge 1 commit into
qdm12:masterfrom
CompewterTutor:feature/netlify-dns-support
Open

feat: Added Netlify as a provider. #1052
CompewterTutor wants to merge 1 commit into
qdm12:masterfrom
CompewterTutor:feature/netlify-dns-support

Conversation

@CompewterTutor

Copy link
Copy Markdown

Added the provider (note…updates aren't supported so we have to delete and then recreate)., created documentation for it and updated the readme. Passes linting though I did have to migrate the lint config to get it to run but those changes are not included in this commit.

…: updates aren't supported so we have to delete and then recreate). Passes linting though I did have to migrate the lint config to get it to run but those changes are not included in this commit.
Comment thread docs/netlify.md
Comment on lines +36 to +41

2. Navigate to **Site settings** for your site

3. Go to **Domain management** → **DNS zones**

4. Ensure your domain is properly configured as a DNS zone in Netlify

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

nit let's use 1. everywhere so github markdown handles the numbering

Suggested change
2. Navigate to **Site settings** for your site
3. Go to **Domain management****DNS zones**
4. Ensure your domain is properly configured as a DNS zone in Netlify
1. Navigate to **Site settings** for your site
1. Go to **Domain management****DNS zones**
1. Ensure your domain is properly configured as a DNS zone in Netlify

Comment thread docs/netlify.md
Comment on lines +42 to +44

## Token setup

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

let's just continue directly to the token setup

Suggested change
## Token setup

Comment thread docs/netlify.md
Comment on lines +46 to +57

2. Click **New access token**

3. Give the token a descriptive name (e.g., "DDNS Updater")

4. Select the following scopes:
- **DNS:read** - Read DNS zones and records
- **DNS:edit** - Edit DNS zones and records

5. Click **Generate token**

6. Copy the generated token - this is your `"token"` value

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
2. Click **New access token**
3. Give the token a descriptive name (e.g., "DDNS Updater")
4. Select the following scopes:
- **DNS:read** - Read DNS zones and records
- **DNS:edit** - Edit DNS zones and records
5. Click **Generate token**
6. Copy the generated token - this is your `"token"` value
1. Click **New access token**
1. Give the token a descriptive name (e.g., "DDNS Updater")
1. Select the following scopes:
- **DNS:read** - Read DNS zones and records
- **DNS:edit** - Edit DNS zones and records
1. Click **Generate token** - this is your `"token"` value

Comment thread docs/netlify.md
Comment on lines +59 to +67
## Testing

1. Go to your Netlify site's DNS management page

2. Check the current DNS record for your domain

3. Run ddns-updater

4. Refresh the Netlify DNS page to verify the update occurred

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I appreciate the effort in documentation, but let's remove this section, I don't think it's really necessary. ddns-updater should log if anything wrong happens.

Suggested change
## Testing
1. Go to your Netlify site's DNS management page
2. Check the current DNS record for your domain
3. Run ddns-updater
4. Refresh the Netlify DNS page to verify the update occurred

Comment thread docs/netlify.md

## Notes

- Netlify's DNS API requires the domain to be configured as a DNS zone in your Netlify account

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this is already mentioned in the domain setup section

Suggested change
- Netlify's DNS API requires the domain to be configured as a DNS zone in your Netlify account

Comment on lines +293 to +296
record, err := p.getDNSRecord(ctx, zoneID, recordID)
if err != nil {
return fmt.Errorf("failed to get existing DNS record: %w", err)
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

do we need to get the record information only to have record.Hostname? Can't we build it locally without doing a request?

It feels like getDNSRecord could be removed

Comment on lines +411 to +415
Priority int64 `json:"priority,omitempty"`
Weight int64 `json:"weight,omitempty"`
Port int64 `json:"port,omitempty"`
Flag int64 `json:"flag,omitempty"`
Tag string `json:"tag,omitempty"`

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

unused so remove them

Suggested change
Priority int64 `json:"priority,omitempty"`
Weight int64 `json:"weight,omitempty"`
Port int64 `json:"port,omitempty"`
Flag int64 `json:"flag,omitempty"`
Tag string `json:"tag,omitempty"`

Type string `json:"type"`
Hostname string `json:"hostname"`
Value string `json:"value"`
TTL int64 `json:"ttl"`

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

what happens if we leave this empty

Suggested change
TTL int64 `json:"ttl"`
TTL int64 `json:"ttl,omitempty"`

does netlify set a default ttl on their own? That might be more appropriate than having a hardcoded default ttl in ddns-updater

}

// DNS record creation structure.
type dnsRecordCreate struct {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

since this is used in only one place, I would recommend declaring this type where it's used inside the method instead of at global scope.

}

// DNS record structure.
type dnsRecord struct {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

since this will be used in only one place, I would recommend declaring this type where it's used inside the method instead of at global scope. You can also remove all fields except ID as far as I recall, other fields are not used.

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.

3 participants