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

Refactor to use database as DNS ground truth, remove DNS worker code #464

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Mar 28, 2023

Fixes #440

This switches over to use the database as our source of truth for all DNS Record info. The reconciler is now responsible for making this happen in Route53, and we don't use DNS workers or direct calls anymore.

I've also fixed the bug where we didn't update the optional parts of a DNS Record in the db while editing.

I have to fix something with e2e tests, but I wanted to get this in fast, since it's blocking testing on Staging.

@humphd humphd self-assigned this Mar 28, 2023
@humphd
Copy link
Contributor Author

humphd commented Mar 28, 2023

There might be more code we can remove that I'm missing. Let me know if you see or think of anything.

Myrfion
Myrfion previously approved these changes Mar 29, 2023
Copy link
Contributor

@Myrfion Myrfion left a comment

Choose a reason for hiding this comment

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

Good to see have simpler things got and that we should care about the record status anymore!

@@ -121,7 +80,6 @@ export default function DnsRecordsTable(props: DnsRecordsTableProps) {
<Table variant="striped" colorScheme="gray">
<Thead>
<Tr>
<Th />
<Th>Subdomain</Th>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change this column name to DNS Record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll do that.

@humphd
Copy link
Contributor Author

humphd commented Mar 29, 2023

Updated to not use Subdomain

Genne23v
Genne23v previously approved these changes Mar 29, 2023
Myrfion
Myrfion previously approved these changes Mar 29, 2023
@humphd
Copy link
Contributor Author

humphd commented Mar 29, 2023

@Eakam1007 because this is so big, I'll get you to do your review too before I merge (and anyone else that wants to--let me know).

@Genne23v Genne23v added category: DNS A service about hosting domains category: data Anything related to data management and structure labels Mar 29, 2023
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.

@humphd

When we are changing a record, I think we should call setReconciliationNeeded(true) from the systemState model
I think this could be integrated into the DNS model

@humphd
Copy link
Contributor Author

humphd commented Mar 29, 2023

@humphd

When we are changing a record, I think we should call setReconciliationNeeded(true) from the systemState model I think this could be integrated into the DNS model

Ah, indeed. Changing now...

@humphd humphd requested a review from a user March 29, 2023 15:52
@humphd humphd dismissed stale reviews from Myrfion and Genne23v via bd93f79 March 29, 2023 15:52
@humphd humphd requested review from Myrfion and Genne23v March 29, 2023 15:52
@humphd
Copy link
Contributor Author

humphd commented Mar 29, 2023

Updated to indicate that reconciler is needed after CRUD operations.

@@ -48,7 +49,11 @@ export async function createDnsRecord(
// Set expiration date 6 months from now
const expiresAt = dayjs().add(6, 'month').toDate();

return prisma.dnsRecord.create({ data: { ...data, expiresAt } });
return prisma.dnsRecord.create({ data: { ...data, expiresAt } }).then((result) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we extract it in a separate function?

(result) => {
      // Flag the reconciler that an update will be needed
      setIsReconciliationNeeded(true);
      return result;
    })

Like flagReconciliationNeeded or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I started to write it, and it's literally the same code. I'm not sure adding a new function does much for us here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like setIsReconciliationNeeded() does the job of setting that flag.

@cychu42
Copy link
Contributor

cychu42 commented Mar 29, 2023

Updated to indicate that reconciler is needed after CRUD operations.

Do we need it for renewDnsRecordById() in app/models/dns-record.server.ts?

ghost
ghost previously approved these changes Mar 29, 2023
@ghost
Copy link

ghost commented Mar 29, 2023

Updated to indicate that reconciler is needed after CRUD operations.

Do we need it for renewDnsRecordById() in app/models/dns-record.server.ts?

No, that is only for our own system, telling when to delete the record

Us deleting the record from db, will trigger the reconciler to delete it from Route53

Eakam1007
Eakam1007 previously approved these changes Mar 29, 2023
Myrfion
Myrfion previously approved these changes Mar 29, 2023
@humphd humphd dismissed stale reviews from Myrfion, Eakam1007, and ghost via 8d90122 March 29, 2023 16:34
@humphd
Copy link
Contributor Author

humphd commented Mar 29, 2023

Rebased and squashed.

@humphd humphd requested review from Myrfion, a user and Eakam1007 March 29, 2023 16:35
@humphd humphd merged commit ad72c19 into DevelopingSpace:main Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: data Anything related to data management and structure category: DNS A service about hosting domains
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace DNS Record status UI/logic
5 participants