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

feat: replace pure SQL strings with SQLair #122

Merged
merged 9 commits into from
Nov 22, 2024
Merged

feat: replace pure SQL strings with SQLair #122

merged 9 commits into from
Nov 22, 2024

Conversation

kayra1
Copy link
Contributor

@kayra1 kayra1 commented Nov 12, 2024

Description

This change addresses the final points and closes #84, which modifies the database to have explicit ID's, adds a new status column and associated SQL checks, and utilizes SQLair to simplify database SQL calls.

It also changes the API of the database to fit the new way of using the status column. We now have functions that are abstracted from the direct SQL calls, like UpdateCSR -> AddCertificateToCSR. This is to simplify future development and help avoid getting the database into inconsistent states.

Another change that SQLair brings is the fact that it no longer returns the Result and LastID values that we used to pass along to the HTTP server. This means that successful API calls only receive a JSON message indicating success instead.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that validate the behaviour of the software
  • I validated that new and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@kayra1 kayra1 marked this pull request as ready for review November 19, 2024 13:25
@kayra1 kayra1 requested a review from a team as a code owner November 19, 2024 13:25
Copy link
Collaborator

@gruyaume gruyaume left a comment

Choose a reason for hiding this comment

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

We use the word CSR to mean two different things here, I made a couple of suggestions to decouple CertificateRequest from CSR, where CSR is the X.509 document and a CertificateRequest is a Notary object that contains a CSR, a Certificate and a Status.

go.mod Show resolved Hide resolved
internal/db/db.go Show resolved Hide resolved
internal/db/db.go Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/server/handlers_certificate_requests_test.go Outdated Show resolved Hide resolved
internal/server/handlers_certificate_requests.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
Copy link
Contributor

@saltiyazan saltiyazan left a comment

Choose a reason for hiding this comment

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

In general this looks good.

I added a few minor comments, and I agree with Guillaume that it makes it easier to read if the Request object and the CSR string have different names.

ui/src/types.ts Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gruyaume gruyaume left a comment

Choose a reason for hiding this comment

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

The sqlair changes look pretty good! Why include Notary HTTP API changes in this PR though? I don't necessarily disagree with those but they don't match the PR description and unnecessarily complexify the PR, and the review process.

internal/server/handlers_accounts.go Show resolved Hide resolved
@kayra1 kayra1 requested a review from gruyaume November 21, 2024 12:49
Copy link
Contributor

@saltiyazan saltiyazan 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, given this comment gets resolved.

@kayra1 kayra1 merged commit 4c3b41f into main Nov 22, 2024
14 checks passed
@kayra1 kayra1 deleted the db branch November 22, 2024 08:58
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.

Ben's style and structure review
3 participants