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

Respond to tony's updates #3

Merged
merged 7 commits into from
Sep 24, 2023
Merged

Respond to tony's updates #3

merged 7 commits into from
Sep 24, 2023

Conversation

benthecarman
Copy link
Contributor

No description provided.

version BIGINT NOT NULL,
created_date TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL,
updated_date TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL,
created_date TIMESTAMP DEFAULT '2023-07-13'::TIMESTAMP NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this. I don't like the idea of the SQL default being here to convert the timestamp. I'm also not really sure how this is supposed to play with the functions you created for setting timestamp. Is there not something built in for this? Why not do the manual timestamp default when inserting data that has None for the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it'd be ideal if we can enforce the NOT NULL on these columns, wanted to have it set so rows we insert from the migration from the old db would get the launch date timestamp

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/routes.rs Outdated Show resolved Hide resolved
@benthecarman benthecarman force-pushed the tony-updates branch 2 times, most recently from 24f7094 to 707313d Compare September 23, 2023 03:38
@@ -0,0 +1,76 @@
CREATE TABLE IF NOT EXISTS vss_db
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why this file is here since this is in the migrations folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah meant to delete

src/models/migration_baseline.sql Outdated Show resolved Hide resolved
@TonyGiorgio
Copy link
Contributor

How are migrations going to be handled going forward without diesel? Have you thought about a way to track that and deal with upgrades?

@benthecarman
Copy link
Contributor Author

How are migrations going to be handled going forward without diesel? Have you thought about a way to track that and deal with upgrades?

We can do migrations with diesel and still have it not in the code, we can do it from the command line

@benthecarman benthecarman merged commit a6a4b6f into master Sep 24, 2023
1 check passed
@benthecarman benthecarman deleted the tony-updates branch September 24, 2023 20: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.

2 participants