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

Add interface to DNS and DB #162

Merged
merged 2 commits into from
Feb 12, 2023
Merged

Conversation

Genne23v
Copy link
Contributor

@Genne23v Genne23v commented Feb 8, 2023

Closes #151
I wrote a long detailed change summary here, but I hit a wrong button and lost it all. This is not a client interface as I thought this is too big to be a direct interface to client. Please let me know if I took a wrong approach.

  • createUserDomain() => Create a record in Route 53 and DB and return created record. If type, name, value exists, it declines to add it to DB.
  • updateUserDomain()
  • deleteUserDomain()
  • removeIfExpired() => Remove a record when it's expired
  • isDomainBeingExpired()
  • isDomainExpired()

Quick high level review would be much appreciated!

@Genne23v Genne23v added this to the Milestone 0.2 milestone Feb 8, 2023
@Genne23v Genne23v requested a review from humphd February 8, 2023 22:15
@Genne23v Genne23v self-assigned this Feb 8, 2023
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.

Nice work @Genne23v. It's great how you're working through hard problems, creating APIs, and writing tests.

I've done a first pass through this code. I'll do more after you've had a chance to fix these.

app/lib/client.server.ts Outdated Show resolved Hide resolved
app/lib/client.server.ts Outdated Show resolved Hide resolved
app/models/record.server.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
app/models/record.server.ts Outdated Show resolved Hide resolved
app/models/record.server.ts Outdated Show resolved Hide resolved
app/lib/client.server.ts Outdated Show resolved Hide resolved
app/models/record.server.ts Outdated Show resolved Hide resolved
app/models/record.server.ts Outdated Show resolved Hide resolved
app/models/record.server.ts Outdated Show resolved Hide resolved
@Genne23v
Copy link
Contributor Author

Genne23v commented Feb 8, 2023

@humphd I thought this might be a lot of rework. It's good to know that I'm on the right path. I will thoroughly review the whole code again to minimize the iteration of review.

@humphd
Copy link
Contributor

humphd commented Feb 8, 2023

I thought this might be a lot of rework. It's good to hear that I'm on the right path. I will thoroughly review the whole code again to reduce the amount of review.

I think it might be useful to hash out APIs in Issues before you dive in, since it means more work for you to change things around. However, I'm also pleased to see you pushing so hard on this DNS code.

I do want to change a bunch of things here, but we can do it starting from what you have. I'd avoid changing the tests too much at this point, until we figure out the right API.

app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/lib/dns.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
app/lib/domains.server.ts Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Feb 10, 2023

Let me know when I should re-review this.

@Genne23v
Copy link
Contributor Author

@humphd Yes, please review my updated PR.

@humphd
Copy link
Contributor

humphd commented Feb 10, 2023

@humphd Yes, please review my updated PR.

When you want someone to re-review, you have to click this refresh button:

Screenshot 2023-02-10 at 3 04 46 PM

@Genne23v Genne23v requested a review from humphd February 10, 2023 20:06
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 really close! A few more simplifications and optimizations.

app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
app/lib/domains.server.ts Outdated Show resolved Hide resolved
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.

Almost there...

test/unit/dns.server.test.ts Outdated Show resolved Hide resolved
test/unit/dns.server.test.ts Outdated Show resolved Hide resolved
test/unit/domains.server.test.ts Outdated Show resolved Hide resolved
test/unit/domains.server.test.ts Outdated Show resolved Hide resolved
test/unit/domains.server.test.ts Outdated Show resolved Hide resolved
test/unit/domains.server.test.ts Show resolved Hide resolved
test/unit/domains.server.test.ts Outdated Show resolved Hide resolved
test/unit/domains.server.test.ts Outdated Show resolved Hide resolved
test/unit/domains.server.test.ts Outdated Show resolved Hide resolved
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.

Few small things, but it looks great. If you want to move this stuff to new issues so we don't block this, that's fine too.

app/lib/dns.server.ts Show resolved Hide resolved
app/lib/domains.server.ts Show resolved Hide resolved
@Genne23v Genne23v requested a review from a user February 12, 2023 13:28
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.

LGTM

@Genne23v Genne23v merged commit 4f5e534 into DevelopingSpace:main Feb 12, 2023
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.

Add wrapper to connect DB and DNS module
2 participants