feat: Add default tll for record

[nobleess]

No description provided.

[mircea-pavel-anton]

Hi, @nobleess, and thank you for your contribution!

I think the default TTL should somehow be configurable via env vars by the user. When you deploy the webhook to your cluster, there should be some configurable option to set it.
What do you think?

I'm currently on my phone so I haven't looked at the code much, but I'll try to get to it tomorrow.

[nobleess]

@mircea-pavel-anton I agree with you. I watched how it was implemented in aws.go and tried to repeat solution here.

[mircea-pavel-anton]

Overall I think this is exactly what we need, just some nitpicks on the code structure 😅

[mircea-pavel-anton]

Also, some documentation change would also be nice for this. We can add a new section under ⚙️ Configuration Options called Default Values for RouterOS Records and document the env var there

[mircea-pavel-anton]

also, CI is failing because of missing bot app credentials, but that is expected.

You can lint your go code using task go:lint and run the unit tests using task go:test. I think the tests will also have to be updated sadly

[mircea-pavel-anton]

I updated the workflows so they can run on forks as well and rebased this branch to get that update. All workflows seem to pass now

[mircea-pavel-anton]

I think I found a new issue here. The check and enforcement of the default ttl is currently only done on record creation. This means that external DNS will create the record with the default ttl correctly, but then at the next loop it will update it back to 0. We should probably do a similar check for updating records

[mircea-pavel-anton]

Also, when we are initializing newChanges a few lines up, we're setting newChanges.Create = changes.Create. This causes the entire list to be duplicated. We need to set:

	newChanges := &plan.Changes{
		Create:    []*endpoint.Endpoint{},
		Delete:    changes.Delete,
		UpdateOld: []*endpoint.Endpoint{},
		UpdateNew: []*endpoint.Endpoint{},
	}

[mircea-pavel-anton]

This all looks good. Now we just need to fix the issue of injecting the default TTL in updates as well.

Basically the behavior I am seeing right now is that when I create a new record it does get the default TTL set, but then on the next reconciliation loop, it gets set back to 0.

I am not entirely sure where/how to address this. Perhaps in isEndpointMatching? I'll see if I can think of something too

[mircea-pavel-anton]

I played around a bit with the issue on a different branch and I think I got a working proof of concept: https://github.com/mirceanton/external-dns-provider-mikrotik/tree/local-test

I haven't tested this extensively, but so far it seems to work. Feel free to adapt any of that code if you think it fits

The tests would also need to be updated. Let me know if you can/want to look into that 😁

[mircea-pavel-anton]

@nobleess that extra condition for the TTLs makes sure that we can also update to/from default ttl from/to other values.

For example, without it we would not be able to update from a TTL of 5s to default TTL and vice versa.

See here some example tests: https://github.com/mirceanton/external-dns-provider-mikrotik/blob/local-test/internal/mikrotik/provider_test.go#L605-L697

[mircea-pavel-anton]

@nobleess is this ready for review?

[mircea-pavel-anton]

looks good!

[mr-borboto]

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀