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

Fixes #23837: Enforce change request and workflow table schema #5233

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Dec 5, 2023

https://issues.rudder.io/issues/23837

We need to add NOT NULL constraint to the change-validation plugin tables, in order to enforce the table schema (until now there does not seem to be a null value that has been written in the table, in that case the migration would fail but the user would have a nice error log). I added database tests.

Note that this PR is very similar to #5232

@clarktsiory clarktsiory requested a review from fanf December 5, 2023 14:16
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory marked this pull request as draft December 5, 2023 14:57
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory marked this pull request as ready for review December 5, 2023 16:53
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

Looks OK once feedback are addressed, I still need to test it on real data

select count(*)
from information_schema.columns
where table_name = $tableName
and column_name = $columnName
Copy link
Member

Choose a reason for hiding this comment

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

]we have a convention of always using . ${Var} for interpolation

where table_name = $tableName
and column_name = $columnName
and is_nullable = 'YES'
""".query[Int].unique.map(_ > 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 method has been written several times.
Shouldn't it be time to put it in a DbCommonMigration or something?

@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

1 similar comment
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

LGTM

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/5233
-- Your faithful QA
Kant merge: "All our knowledge begins with the senses, proceeds then to the understanding, and ends with reason. There is nothing higher than reason."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/77381/console)

@fanf
Copy link
Member

fanf commented Dec 12, 2023

OK, squash merging this PR

@fanf fanf force-pushed the bug_23837/enforce_change_request_and_workflow_table_schema branch from 80b3009 to a900c33 Compare December 12, 2023 08:49
@fanf fanf merged commit a900c33 into Normation:branches/rudder/7.3 Dec 12, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants