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

fix(migration): Check if column exits before adding it #48480

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Oct 1, 2024

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label Oct 1, 2024
@solracsf solracsf added this to the Nextcloud 31 milestone Oct 1, 2024
@solracsf
Copy link
Member Author

solracsf commented Oct 1, 2024

/backport to stable30

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Makes sense! Can we somehow add a test for this so that such an if clause is never forgotten?

@solracsf solracsf self-assigned this Oct 1, 2024
Copy link
Member

@joshtrichards joshtrichards left a comment

Choose a reason for hiding this comment

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

Check makes sense, but I don't understand the state people are ending up in in that bug report: upgrading from v29 directly shouldn't have that column anyhow.

@solracsf solracsf merged commit b30ef38 into master Oct 1, 2024
173 checks passed
@solracsf solracsf deleted the checkColExists branch October 1, 2024 13:17
@joshtrichards
Copy link
Member

joshtrichards commented Oct 1, 2024

Makes sense! Can we somehow add a test for this so that such an if clause is never forgotten?

Integration testing focused on the the upgrade path should catch these (which we don't have AFAIK). Particularly if we had a matrix covering major->major, beta->major, rc->major, etc.

@solracsf
Copy link
Member Author

solracsf commented Oct 1, 2024

Makes sense! Can we somehow add a test for this so that such an if clause is never forgotten?

Integration testing focused on the the upgrade path should catch these (which we don't have AFAIK). Particularly if we had a matrix covering major->major, beta->major, rc->major, etc.

Yet this is out of the scope of this specific PR, I fully agree on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants