Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

bug(scheduler): Fix controller id length check #2226

Merged
merged 5 commits into from
Jun 22, 2022
Merged

Conversation

louisruch
Copy link
Collaborator

The job_run table has a foreign key to the server_controller private_id.
While this column is called a prviate_id it does not implement the wt_private_id
postgres domain type, and therefore does not have a minimum length of 10 characters.

The job_run table has a foreign key to the server_controller private_id.
While this column is called a prviate_id it does not implement the wt_private_id
postgres domain type, and therefore does not have a minimum length of 10 characters.
ddebko
ddebko previously approved these changes Jun 22, 2022
Copy link
Collaborator

@ddebko ddebko left a comment

Choose a reason for hiding this comment

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

LGTM

jefferai
jefferai previously approved these changes Jun 22, 2022
Copy link
Member

@jefferai jefferai 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; would you mind starting a new Changelog section for 0.9.1?

@jefferai jefferai added this to the 0.9.1 milestone Jun 22, 2022
@louisruch louisruch dismissed stale reviews from jefferai and ddebko via 6dd1718 June 22, 2022 05:31
Comment on lines 13 to 16
add constraint controller_id_must_not_be_empty
check(
length(trim(controller_id)) > 0
);
Copy link
Member

Choose a reason for hiding this comment

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

This constraint should also be on the server_controller table since controller_id is a foreign key. Can we create a domain type for this constraint and then change the column types in the server_controller and job_run tables to use the new domain type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was not sure on what the domain type should be, so I went with wt_not_empty let me know what you think, otherwise I can make it specific to the controller private id. wt controller_id

talanknight
talanknight previously approved these changes Jun 22, 2022
Comment on lines 20 to 21
alter table job_run
alter column controller_id type wt_not_empty;
Copy link
Member

Choose a reason for hiding this comment

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

Can we combine the changes to the job_run table into one alter table statement since they are related?

alter table job_run
drop constraint controller_id_must_be_at_least_10_characters;

create domain wt_not_empty as text
Copy link
Member

Choose a reason for hiding this comment

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

I think wt_controller_id would be a clearer name and it would allow us to alter the checks or add new ones without changing the name of the domain type.

alter table job_run
drop constraint controller_id_must_be_at_least_10_characters;

create domain wt_not_empty as text
Copy link
Member

Choose a reason for hiding this comment

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

Can we add sql tests for this?

@louisruch louisruch requested a review from mgaffney June 22, 2022 18:03
@louisruch louisruch merged commit 4cacd0a into main Jun 22, 2022
@louisruch louisruch deleted the louis-scheduler branch June 22, 2022 20:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants