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

catch error if no dns domains exist #34605

Merged
merged 1 commit into from
Jul 14, 2016
Merged

Conversation

gtmanfred
Copy link
Contributor

@gtmanfred gtmanfred commented Jul 12, 2016

What does this PR do?

catch error if no dns domains exist for digital ocean.

What issues does this PR fix or reference?

Fixes #33452

Tests written?

No

@rallytime
Copy link
Contributor

@gtmanfred Can you fix the lint error please?

@rallytime rallytime added the pending-changes The pull request needs additional changes before it can be merged label Jul 13, 2016
try:
response = query(method='domains', droplet_id=domain, command='records')
except SaltCloudSystemExit
log.debug('Failed to find domains.')
Copy link
Contributor

@rallytime rallytime Jul 13, 2016

Choose a reason for hiding this comment

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

Do we really only want to log this at a debug level and then return? That might make debugging hard when the response fails legitimately.

This also brings up another point - do we want to be calling the destroy_dns_records function if delete_dns_record: True is not set in the digital ocean config files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is fair, lemme try something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, the reason we always attempt to delete it is because you cannot know the profile that a server was built with. The vm_ data when deleting servers does not have any profile data, it only has the provider data, so because you can't be that granular, it is assumed that stale dns records are bad.

This is noted in the code in line 754.

    # TODO: when _vm config data can be made available, we should honor the configuration settings,
    # but until then, we should assume stale DNS records are bad, and default behavior should be to
    # delete them if we can. When this is resolved, also resolve the comments a couple of lines below.

So I would think that we should do this log and always try to delete.

@rallytime rallytime added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jul 13, 2016
try:
response = query(method='domains', droplet_id=domain, command='records')
except SaltCloudSystemExit:
log.debug('Failed to find domains.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be at an error level or debug?

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 am ok with either, but as this is run every time and can't be turned off I had it as debug so it wouldn't show up all the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha.

@cachedout cachedout merged commit b88c39e into saltstack:2016.3 Jul 14, 2016
@ahammond
Copy link
Contributor

@rallytime will this be in .2? Please please please?!?!

@gtmanfred
Copy link
Contributor Author

It will be

On Thu, Jul 14, 2016 at 4:43 PM Andrew Hammond notifications@github.com
wrote:

@rallytime https://github.com/rallytime will this be in .2? Please
please please?!?!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34605 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAssocCU-0sRX8qc4-C3CHmxZFtSJNOaks5qVq1jgaJpZM4JKfOb
.

@rallytime
Copy link
Contributor

@ahammond yep! It will be :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-changes The pull request needs additional changes before it can be merged Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants