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.

Sign up for GitHub

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

Jump to bottom

Add Worker/Queue to process expired DNS records #357

Merged
merged 1 commit into from
Mar 18, 2023
Merged

Add Worker/Queue to process expired DNS records #357

merged 1 commit into from
Mar 18, 2023

Conversation

SerpentBytes
Copy link
Contributor

Description

Resolves #112 by adding Worker/Queue to do the following:

  1. Query the database for expired DNS records
  2. If Step 1. yields any results, delete DNS Records from Route53 and delete the record from DB (via deleteDnsRequest)
  3. Send Notification

Step 2 is not working as intended. Invoking deleteDnsRequest in the front-end/back-end fails with the following warning:

warn: Invalid request: Expected exactly one of [AliasTarget, all of [TTL, and ResourceRecords], or TrafficPolicyInstanceId], but found none in Change with [Action=DELETE, Name=osd700-a1.user1.starchart.com, Type=A, SetIdentifier=null]

Possibly linked to the missing TTL value.

@SerpentBytes SerpentBytes requested a review from humphd March 16, 2023 18:27
@SerpentBytes SerpentBytes self-assigned this Mar 16, 2023
@SerpentBytes SerpentBytes added category: DNS A service about hosting domains category: notifications A service to notify users about their certificates/domains labels Mar 16, 2023
humphd
humphd requested changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is looking good. A few things I noticed on first reading.

app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
@SerpentBytes SerpentBytes force-pushed the issue-112 branch 2 times, most recently from dde70fc to ebe1956 Compare March 17, 2023 16:49
@SerpentBytes
Copy link
Contributor Author

@humphd, I have updated the code based on your initial feedback.

@SerpentBytes SerpentBytes requested a review from humphd March 17, 2023 16:52
humphd
humphd requested changes Mar 17, 2023
View reviewed changes
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
humphd
humphd requested changes Mar 17, 2023
View reviewed changes
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
@SerpentBytes SerpentBytes marked this pull request as ready for review March 17, 2023 17:34
@SerpentBytes SerpentBytes marked this pull request as draft March 17, 2023 17:53
@SerpentBytes
Copy link
Contributor Author

@humphd I have removed the if check and updated the notifications job-related .env variables values so casting to Number from String does not result in NaN.

@SerpentBytes SerpentBytes requested a review from humphd March 17, 2023 19:48
@SerpentBytes SerpentBytes marked this pull request as ready for review March 17, 2023 20:01
@SerpentBytes SerpentBytes requested review from a user, Genne23v and sfrunza13 March 17, 2023 20:10
humphd
humphd requested changes Mar 18, 2023
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Looks good. A few things I notice and I think this is ready.

.env.example Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
@SerpentBytes SerpentBytes requested a review from humphd March 18, 2023 12:37
@SerpentBytes
Copy link
Contributor Author

SerpentBytes commented Mar 18, 2023 •
edited
Loading

Updated the code with helpful comments and added the if condition to return if we get null/undefined back from the query that fetches expired DNS records.

Edit: Updated environment variables names to be more precise.

@SerpentBytes SerpentBytes added this to the Milestone 0.6 milestone Mar 18, 2023
@SerpentBytes SerpentBytes mentioned this pull request Mar 18, 2023
Closed
ghost
ghost suggested changes Mar 18, 2023
View reviewed changes
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
humphd
humphd requested changes Mar 18, 2023
View reviewed changes
.env.example Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
app/queues/common/expiration-request.server.ts Outdated Show resolved Hide resolved
@SerpentBytes SerpentBytes marked this pull request as draft March 18, 2023 15:49
@SerpentBytes SerpentBytes requested review from a user and humphd March 18, 2023 16:37
@SerpentBytes SerpentBytes marked this pull request as ready for review March 18, 2023 16:38
humphd
humphd previously approved these changes Mar 18, 2023
View reviewed changes
ghost
ghost previously approved these changes Mar 18, 2023
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Regardless, I'm going to approve this, if we decide to, we can make that change later, or you can incorporate it int this pr as well. Whatever makes more sense to you.

@SerpentBytes SerpentBytes dismissed stale reviews from ghost and humphd via 9d60980 March 18, 2023 20:20
@SerpentBytes
Copy link
Contributor Author

SerpentBytes commented Mar 18, 2023 •
edited
Loading

@dadolhay I misunderstood you earlier. I have updated the code to process DNS records expiration without using type.

@SerpentBytes SerpentBytes requested review from a user and humphd March 18, 2023 20:24
@SerpentBytes SerpentBytes marked this pull request as draft March 18, 2023 20:43
@SerpentBytes
Add Worker/Queue to process expired DNS records
120c846 
* Add code skeleton
* Add switch statement and log messages
* Add Worker/Queue to process DNS records
* Add auto job removal on completion/failure
* Fix typo in removeOnFail
* Fix: eslint warnings
* Unexpose init()
* Update code based on feedback
* Resolve merge conflict
* fix: pass name as subdomain to deleteDnsRequest
* use result value instead of expression
* remove: if check for possiblly undefined value
* Uncomment needed code
* Update code based on feedback
* Make variable names consistent with .env.example
* Update variable name, add comments, move query func to /models
* use single queue; remove test log
* use local constant
* remove pointless validation for constant being undefined
* Update code based on feedback
* Update code based on feedback
* fix: eslint warning regarding unused import
* re-position log message
* fix: typecheck
@SerpentBytes SerpentBytes marked this pull request as ready for review March 18, 2023 20:53
ghost
ghost approved these changes Mar 18, 2023
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks awesome!

@SerpentBytes SerpentBytes merged commit 8f7eeb1 into DevelopingSpace:main Mar 18, 2023
@SerpentBytes SerpentBytes deleted the issue-112 branch March 18, 2023 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humphd humphd humphd approved these changes

@Genne23v Genne23v Awaiting requested review from Genne23v

@sfrunza13 sfrunza13 Awaiting requested review from sfrunza13

Assignees

@SerpentBytes SerpentBytes

Labels
category: DNS A service about hosting domains category: notifications A service to notify users about their certificates/domains
Projects
None yet
Milestone
Milestone 0.6
Development

Successfully merging this pull request may close these issues.

Expire DNS records after a configurable amount of time
2 participants
@SerpentBytes @humphd