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

Support for initial healthcheck dynamic record #53

Merged
merged 7 commits into from
Jun 20, 2024
Merged

Conversation

gael-api
Copy link
Contributor

@gael-api gael-api commented May 2, 2024

Hi,

Here's my first contribution to OctoDNS.
We use health check dynamic monitoring with Gcore DNS.
The support is quite minimal and managed in parallel of original geodns dynamic record.

Test coverage is 100% done.


Copy link
Contributor

@ross ross 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 updates. Comments lnline. I don't have access to or knowledge of gcore itself so most of my comments are centered around octodns and its practices. Someone more familiar with gcore should probably 👀 over it as well.

octodns_gcore/__init__.py Outdated Show resolved Hide resolved
octodns_gcore/__init__.py Show resolved Hide resolved
octodns_gcore/__init__.py Outdated Show resolved Hide resolved
octodns_gcore/__init__.py Outdated Show resolved Hide resolved
@@ -594,6 +708,7 @@ def _apply_update(self, change):
self.log.info("updating: %s", change)
new = change.new
data = getattr(self, f"_params_for_{new._type}")(new)
self.log.debug("updating: %s", str(data))
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a test/debugging log message rather than something targeted at/useful for end users. Intentionally left in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's a debugging log. It was helpful during the dev but can be safely removed.

@istr
Copy link
Contributor

istr commented May 5, 2024

Thanks for the updates. Comments lnline. I don't have access to or knowledge of gcore itself so most of my comments are centered around octodns and its practices. Someone more familiar with gcore should probably 👀 over it as well.

I can have a look and test against my GCore account.

@ross
Copy link
Contributor

ross commented May 31, 2024

@istr did you get a chance to 👁️ this one?

Looks like there's at least a couple minor things left to address from my original feedback.

* add _GcoreDynamicRecord

* Move Gcore specific to healthcheck subkey
@gael-api
Copy link
Contributor Author

gael-api commented Jun 3, 2024

As requested, I moved the gcore specific healthcheck parameters to a subkey.

Copy link
Contributor

@ross ross left a comment

Choose a reason for hiding this comment

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

I think my suggestions have been addressed. Otherwise relying on you (all) to verify that things work as needed/intended since I don't have access.

@ross ross merged commit 43cb9c1 into octodns:main Jun 20, 2024
7 checks passed
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