Skip to content

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

Merged
solracsf merged 1 commit into
masterfrom
checkColExists
Oct 1, 2024
Merged

fix(migration): Check if column exits before adding it#48480
solracsf merged 1 commit into
masterfrom
checkColExists

Conversation

@solracsf

@solracsf solracsf commented Oct 1, 2024

Copy link
Copy Markdown
Member

Checklist

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@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

solracsf commented Oct 1, 2024

Copy link
Copy Markdown
Member Author

/backport to stable30

@szaimen szaimen left a comment

Copy link
Copy Markdown
Contributor

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

@joshtrichards joshtrichards left a comment

Copy link
Copy Markdown
Member

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.

@joshtrichards

joshtrichards commented Oct 1, 2024

Copy link
Copy Markdown
Member

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

solracsf commented Oct 1, 2024

Copy link
Copy Markdown
Member Author

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

Development

Successfully merging this pull request may close these issues.

[Bug]: Exception: Database error when running migration The column "scheduled_at" on table "oc_taskprocessing_tasks" already exists.

3 participants