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

Enable CI builds with postgresql and mariadb #631

Merged
merged 2 commits into from
Jul 9, 2021

Conversation

zorun
Copy link
Collaborator

@zorun zorun commented May 24, 2020

This is still a WIP to see if it works.

@zorun zorun changed the title WIP: enable CI builds with postgresql WIP: enable CI builds with postgresql and mysql May 24, 2020
@indatwood
Copy link
Contributor

Hey @zorun what should we do with this? Was it working? Should we merge it, or close it? Thanks for the heads up!

@zorun
Copy link
Collaborator Author

zorun commented Feb 4, 2021

Hey @zorun what should we do with this? Was it working? Should we merge it, or close it? Thanks for the heads up!

Hey, sorry for leaving this in a WIP state. I just rebased it (it needed some manual fix-up).

As far as I remember, it wasn't merged because the tests were failing with mysql (see #632). We need to fix that issue before merging, otherwise all PRs would appear to be faulty.

@zorun
Copy link
Collaborator Author

zorun commented Feb 4, 2021

This should fail (because I explictly trigger a known bug): https://travis-ci.org/github/spiral-project/ihatemoney/builds/757617787

This is supposed to work but will probably fail because of #632 : https://travis-ci.org/github/spiral-project/ihatemoney/builds/757618257

@almet
Copy link
Member

almet commented Apr 7, 2021

Hey, if the tests are failing with this, and it's the expected behavior, then I think we should merge it and fix the bug that this points out. Right?

So, merge time?

@almet almet force-pushed the ci_pgsql branch 4 times, most recently from 73507cd to 15adc2e Compare April 11, 2021 17:17
@zorun
Copy link
Collaborator Author

zorun commented Apr 11, 2021

Hey, if the tests are failing with this, and it's the expected behavior, then I think we should merge it and fix the bug that this points out. Right?

So, merge time?

Well, it depends if fixing the MySQL bug takes 1 day or 1 year :)

If we merge this while not fixing the bug, all pull requests will have failing CI. It's not very nice for people proposing patches.

@Glandos
Copy link
Member

Glandos commented Apr 12, 2021

Is it possible to replace service: mysql with mariadb to test it as noted in #632 (comment) ?

@zorun zorun force-pushed the ci_pgsql branch 8 times, most recently from 82f8175 to 0d7e883 Compare July 4, 2021 18:20
@zorun zorun force-pushed the ci_pgsql branch 2 times, most recently from 8efe921 to 48fadcd Compare July 4, 2021 18:37
@zorun zorun changed the title WIP: enable CI builds with postgresql and mysql Enable CI builds with postgresql and mariadb Jul 4, 2021
@zorun
Copy link
Collaborator Author

zorun commented Jul 4, 2021

I've added support for the github CI, and switched to mariadb instead of mysql.

I'm not 100% satisfied, because the postgresql and mysql containers are started in all jobs (even those using sqlite). But it only means that the CI is a bit longer than necessary, and fixing it would involve writing multiple actions with some kind of inheritance, and I'm too lazy to learn how to do that.

So, it's good to go for a review @Glandos !

@zorun zorun requested a review from Glandos July 4, 2021 18:41
.travis.yml Outdated Show resolved Hide resolved
Copy link
Member

@Glandos Glandos left a comment

Choose a reason for hiding this comment

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

See comment on .travis.yml

@zorun
Copy link
Collaborator Author

zorun commented Jul 7, 2021

Uh, weird, I just rebased and there's a new postgresql error:

psycopg2.errors.GroupingError: column "bill.id" must appear in the GROUP BY clause or be used in an aggregate function

@zorun
Copy link
Collaborator Author

zorun commented Jul 7, 2021

Ah, it's the new check from #661 . I should have merged before you @Glandos ^^

return self.get_bills_unordered().group_by(Bill.original_currency).count() > 1

I'm looking at it.

@zorun zorun merged commit 7ceb66f into spiral-project:master Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants