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

Initial schema #79

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Initial schema #79

merged 1 commit into from
Feb 3, 2023

Conversation

cychu42
Copy link
Contributor

@cychu42 cychu42 commented Feb 1, 2023

Closes #16
Closes #59

This is the initial schema for the MySQL database. This includes Users, Subdomains, Certificates, and Challenges models.
Let me know if something needs to be changed or added.

@humphd humphd requested a review from a user February 1, 2023 22:41
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 looks really good, great work. I've made a few comments to improve, but it's close.

name String
type SubdomainType
value String
description String
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 that description, course, and ports should all be marked optional (i.e., add a ?). The rest can be required.

prisma/schema.prisma Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
}

enum SubdomainType {
CNAME
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alphabetize these, and put A first.

prisma/schema.prisma Show resolved Hide resolved
username String @id
name String
email String @unique
model Users {
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing I missed before. In Prisma, the suggestion is to use singular vs. plural forms for the names:

  • User vs. Users
  • Subdomain vs. Subdomains
  • etc

@cychu42 cychu42 force-pushed the initSchema branch 2 times, most recently from 6877636 to 16c9130 Compare February 2, 2023 14:25
@cychu42
Copy link
Contributor Author

cychu42 commented Feb 2, 2023

Thank you! I made some changes according to the feedback.

}

model Challenge {
id Int @id @default(autoincrement())
Copy link

Choose a reason for hiding this comment

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

Suggested change
id Int @id @default(autoincrement())
id Int @id @default(autoincrement())
domain String @db.VarChar(255)

Could you please add this?

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.

certificates Certificate?
}

model Subdomain {
Copy link

Choose a reason for hiding this comment

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

I'm almost certain that this should be called Record because multiple records can have the same subdomain

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I think Record is a logical name here, since that's what these are in Route53.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. We just need to make sure this is the name we all use outside of user-facing areas, like UI.
Otherwise, it will get confusing.

* Use singular for model names
* Arrange enum alphabetically
* Make description, course, and ports optional
* Add orderURL and domain columns
@cychu42 cychu42 requested a review from a user February 2, 2023 19:44
@cychu42
Copy link
Contributor Author

cychu42 commented Feb 2, 2023

The code is updated according to feedback.

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.

We'll need to update prisma/seed.ts (and probably some other code) to match this new schema in a follow up.

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.

Create a schema for Prisma Create a certificate table in the database
2 participants