Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

CI: Test migrations #596

Merged
merged 1 commit into from
Jul 22, 2023
Merged

CI: Test migrations #596

merged 1 commit into from
Jul 22, 2023

Conversation

4ydan
Copy link
Contributor

@4ydan 4ydan commented Jul 9, 2023

#134

Basics

  • I added a line to /doc/CHANGELOG.md
  • The PR is rebased with current master.
  • Details of what you changed are in commit messages.
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildserver is happy.

Checklist

  • I have installed and I am using pre-commit hooks
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • I fixed all affected decisions
  • I added unit tests for my code
  • I added code comments, logging, and assertions as appropriate
  • I translated all strings visible to the user
  • I mentioned every code or binary not directly written or done by me in reuse syntax
  • I created left-over issues for things that are still to be done
  • Code is conforming to our Architecture
  • Code is conforming to our Guidelines
    (exceptions are documented)
  • Code is consistent to our Design Decisions

Review

  • I've tested the code
  • I've read through the whole code
  • Examples are well chosen and understandable

@4ydan 4ydan changed the title Test migrations in PR and Master stage CI: Test migrations in PR and Master stage Jul 9, 2023
@4ydan 4ydan added the CI label Jul 9, 2023
@4ydan 4ydan self-assigned this Jul 9, 2023
@4ydan 4ydan marked this pull request as draft July 9, 2023 18:01
@4ydan 4ydan mentioned this pull request Jul 10, 2023
4 tasks
@4ydan 4ydan force-pushed the feature/134-migration-redo branch from 185e3e7 to b60a888 Compare July 13, 2023 18:58
@4ydan 4ydan linked an issue Jul 13, 2023 that may be closed by this pull request
1 task
@4ydan 4ydan changed the title CI: Test migrations in PR and Master stage CI: Test migrations Jul 16, 2023
@4ydan 4ydan force-pushed the feature/134-migration-redo branch 2 times, most recently from 4052a70 to 532e9b1 Compare July 18, 2023 14:58
@4ydan 4ydan marked this pull request as ready for review July 18, 2023 14:58
@4ydan 4ydan requested review from markus2330 and lukashartl July 18, 2023 16:45
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Good to finally get this! 🚀

ci/Jenkinsfile Outdated Show resolved Hide resolved
@4ydan 4ydan force-pushed the feature/134-migration-redo branch from 214691b to bd2e46d Compare July 19, 2023 16:25
@4ydan
Copy link
Contributor Author

4ydan commented Jul 19, 2023

Can we do the migration check before building?

@markus2330
Copy link
Contributor

Probably yes, but I don't know.

@4ydan 4ydan force-pushed the feature/134-migration-redo branch from 434fb01 to c0457a7 Compare July 19, 2023 17:20
@4ydan
Copy link
Contributor Author

4ydan commented Jul 19, 2023

image
Ok so now we need proper rights, is that to be fixed in a separate PR and we wait for the merge?
Or how should I modify the script in CI.

@markus2330
Copy link
Contributor

Ok so now we need proper rights, is that to be fixed in a separate PR and we wait for the merge?

I don't think it is a permission problem. It rather looks like the migrations are broken: the PostGIS extension is tried to be removed but not yet all objects that use PostGIS are removed. (At least I interpret the error message like this.)

I tried to reproduce and got another problem. I created #674 to track these issues.

Probably, we need to wait for #644 (or even longer) until these problems are resolved...

@markus2330
Copy link
Contributor

jenkins build please

@markus2330
Copy link
Contributor

Let us see if #644 improved the situation...

@markus2330
Copy link
Contributor

jenkins build please

@markus2330
Copy link
Contributor

We again have Failed to run 2023-07-01-152628_squashed_setup with: cannot drop extension postgis because other objects depend on it here. I cannot reproduce it locally. Maybe it is a permission problem after all?

@temmey do you maybe know what this error is?

@4ydan 4ydan force-pushed the feature/134-migration-redo branch from 40e4ee6 to ad98477 Compare July 22, 2023 09:28
@4ydan
Copy link
Contributor Author

4ydan commented Jul 22, 2023

Changing DROP EXTENSION postgis; to DROP EXTENSION postgis CASCADE; solves the problem, but not sure if we want this. I also can't reproduce it locally.

@markus2330
Copy link
Contributor

Great that you found a way!

According to https://www.postgresql.org/docs/current/sql-dropextension.html

Automatically drop objects that depend on the extension, and in turn all objects that depend on those objects (see Section 5.14).

As we also reset the whole database on pr builds, afaik there cannot be any harm be removing any objects in the database. On dev (master) or production builds, it might be a problem but there we don't test redo -a anyway and then it doesn't occur, does it?

@4ydan
Copy link
Contributor Author

4ydan commented Jul 22, 2023

Yes I agree, but somehow it still bothers me that there is something.

it might be a problem but there we don't test redo -a anyway and then it doesn't occur, does it?

True, it only occured with make migration-redo-a.

@4ydan
Copy link
Contributor Author

4ydan commented Jul 22, 2023

I guess it will help more than it could do bad. Because finding the issue for that error might take some time in which other PR's could fail migrations maybe? Maybe we can leave an open issue that aims to remove the CASCADE again at some point?

doc/CHANGELOG.md Outdated Show resolved Hide resolved
@markus2330
Copy link
Contributor

Yes, let us merge this, a rebase is needed, though.

I don't think an issue is needed. It is okay if data gets lost on pr or dev.

@4ydan 4ydan force-pushed the feature/134-migration-redo branch from 0bc935b to c6fd7b9 Compare July 22, 2023 13:04
@4ydan 4ydan force-pushed the feature/134-migration-redo branch from c6fd7b9 to 092c18b Compare July 22, 2023 13:07
@4ydan
Copy link
Contributor Author

4ydan commented Jul 22, 2023

Ready to be merged.

@markus2330 markus2330 merged commit 91545ee into master Jul 22, 2023
@markus2330 markus2330 deleted the feature/134-migration-redo branch July 22, 2023 14:10
@markus2330
Copy link
Contributor

Thank you! ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test migrations in CI
2 participants