Skip to content
This repository has been archived by the owner on Jul 16, 2020. It is now read-only.

Refresh zone after record is deleted #2

Merged
merged 6 commits into from
Mar 29, 2018

Conversation

sauerj
Copy link
Contributor

@sauerj sauerj commented Mar 27, 2018

After the ACME challenge is removed from the zone, the new zone data
must be applied to the DNS servers.

After the ACME challenge is removed from the zone, the new zone data
must be applied to the DNS servers.
A syntax and variable name error messed up the refresh action.
@sauerj
Copy link
Contributor Author

sauerj commented Mar 27, 2018

Sorry, I messed up this pull request. Fixes are on the way.

@sauerj
Copy link
Contributor Author

sauerj commented Mar 27, 2018

Now everything should work.

process.exit(1);
}
});

process.exit(0);
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you again for contributing!
Unfortunately it doesn't work yet, the async POST call to refresh the zone never happens because process.exit(0) is called before. You could simply delete that line, since it's completely redundant. Also, I personally like to use different exit codes, so I know where the process exited. You could simply go up a number and use process.exit(3) instead of process.exit(1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I am completely new to node.js...
I see, the POST call is actually inside the DELETE call. That was not what I intended to do.

About the exit code, in 'bin/create_record.js' you used process.exit(1) for exiting when a error occurred. I overlooked that you use different exit codes in 'delete_record.js'.

Copy link
Owner

Choose a reason for hiding this comment

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

We've all been there! ;)

Well, it is actually good the way you did it. You have to wait for the call back from the DELETE call.
The problem is that ovh.request('POST',... puts an async operation on the event loop to not block the thread, meaning it continues the flow, hitting process.exit(0) and closing the whole program all together

Copy link
Owner

Choose a reason for hiding this comment

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

I think simply removing process.exit(0); will be enough.

To ensure that the zone refresh call is actually executed, call it from
an own request.
To distinguish exit point, change exit code when the zone refresh
command fails.
});

process.exit(0);
Copy link
Owner

Choose a reason for hiding this comment

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

Well, now this is even worse 😅
Basically what will happen is:

  1. DELETE request is scheduled
  2. POST request is scheduled
  3. Process exited (0)
  4. DELETE & POST request are executed at the same time.

Of course 4 never happens because the process is already exited. The way it was done before it was perfect. Before, the flow was:

  1. DELETE is scheduled
  2. DELETE is executed
  3. Callback of DELETE is executed
  4. POST is scheduled
  5. Process is exited (0)
  6. POST is executed.

The problem here was simply that the process exited before the refresh call could be executed. I suggest to go back to the previous commits, simply deleting the process.exit(0); line that is superfluous and actually bad.

Copy link
Contributor Author

@sauerj sauerj Mar 29, 2018

Choose a reason for hiding this comment

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

Okay, to be clear: you wrote the process.exit(0); not me. That actually confused me a bit.
What confuses me a little bit more, the code actually works fine. :D

Copy link
Owner

Choose a reason for hiding this comment

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

I know, I wrote it, but when I did there was nothing else to do… while now you have also refresh the zone — which btw is a good idea because it is a necessary step to update the zone.

In my tests it actually doesn't delete the record… you can test by running dig against the DNS record or using the API console here: https://api.ovh.com/console/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I have to excuse. You are right, the record will not be deleted. It is possible that I ran the test with older code... oops.

This reverts commit fa10803.

Commit fa10803 made the situation much worth. We have to wait before
the delete request ist executed before the refresh request can be
sscheduled.
We have to wait until the delete request is finished before exiting the
process.
@mcdado mcdado merged commit 1504a9d into mcdado:master Mar 29, 2018
@mcdado
Copy link
Owner

mcdado commented Mar 29, 2018

Thank you again for your contributions!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants