Skip to content

Conversation

@schew2381
Copy link
Contributor

@schew2381 schew2381 commented Mar 19, 2025

Summary

Hey there, I noticed in #301 that the logic for handling adding new fields as null was split out from adding-not-nullable-field into adding-required-field.

The logic for performing the pg version check wasn't removed however, so this causes the rule to fail to detect changing existing columns to be not null. All the logic for checking not null for adding cols was extracted to adding-required-field or covered by adding-required-field, so I think we can remove the version check altogether.

Examples

#301 has nice examples showing how the other rules cover the adding column with not null. I've included the part below to show what changes with this PR.

Previously with --pg-version >=12:

ALTER TABLE foo ALTER COLUMN d SET NOT NULL; -- no warning,

This PR with --pg-version >=12:

ALTER TABLE foo ALTER COLUMN d SET NOT NULL; -- triggers adding-not-nullable-field

Documentation

I updated adding-not-nullable-field to explicitly mention that it no longer covers adding cols, and added to the problem+solution sections as well.

I think ideally it'd be best to maybe re-name this rule to clarify the fact that it only covers modifying existing cols now, but that may be hard to do


Please let me know if this change may not be the best approach, or if there are any concerns!

@netlify
Copy link

netlify bot commented Mar 19, 2025

👷 Deploy request for squawkhq pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 0987e4f

@schew2381 schew2381 changed the title Change adding-not-nullable field rule to ignore postgres history Change adding-not-nullable field rule to ignore postgres version Mar 19, 2025
@schew2381 schew2381 changed the title Change adding-not-nullable field rule to ignore postgres version Change adding-not-nullable-field to ignore postgres version Mar 19, 2025
@schew2381 schew2381 changed the title Change adding-not-nullable-field to ignore postgres version Ignore postgres version in adding-not-nullable-field Mar 19, 2025
@sbdchd
Copy link
Owner

sbdchd commented Mar 20, 2025

Thank you, this looks good!

I'm going to try and fix CI, I think the actions are deprecated or something

sbdchd added a commit that referenced this pull request Mar 20, 2025
trying to unblock: #412

I think actions/checkout@v3 is deprecated or something
@sbdchd sbdchd added the automerge automerge with kodiak label Mar 20, 2025
@kodiakhq kodiakhq bot merged commit 360e3ee into sbdchd:master Mar 20, 2025
18 of 22 checks passed
sbdchd added a commit that referenced this pull request May 30, 2025
I think the changes from #412 /
360e3ee
didn't make it into the v2 codebase.

rel: #519
kodiakhq bot pushed a commit that referenced this pull request May 30, 2025
…#520)

I think the changes from #412 / 360e3ee didn't make it into the v2 codebase.

rel: #519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge automerge with kodiak

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants