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

initiate usage of pg #319

Merged

Conversation

@TomasTomecek
Copy link
Contributor

TomasTomecek commented Jan 9, 2020

This PR doesn't add any functionality. I'm adding "dead-code" pretty much. On the other hand:

  • it should make reviewing other pg-related PRs trivial
  • it will be easy for everyone to start moving data from redis to pg

Read more about alembic here: https://alembic.sqlalchemy.org/en/latest/cookbook.html#building-uptodate

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Jan 9, 2020

Build failed.

@TomasTomecek TomasTomecek force-pushed the TomasTomecek:use-pg branch 2 times, most recently from fa46818 to 10f4743 Jan 9, 2020
@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Jan 9, 2020

Build failed.

@TomasTomecek TomasTomecek force-pushed the TomasTomecek:use-pg branch from 10f4743 to 361f029 Jan 10, 2020
@TomasTomecek

This comment has been minimized.

Copy link
Contributor Author

TomasTomecek commented Jan 10, 2020

I just tested this locally in a cluster and it works fine.

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Jan 10, 2020

Build failed.

Copy link
Member

lachmanfrantisek left a comment

I haven't tested that locally, but the models look good.

id = Column(Integer, primary_key=True) # our database PK
pr_id = Column(
String, index=True
) # GitHub PR ID - let's not make this PK since we can't control it

This comment has been minimized.

Copy link
@lachmanfrantisek

This comment has been minimized.

Copy link
@TomasTomecek

TomasTomecek Jan 10, 2020

Author Contributor

bear in mind that the models are really just a first pass, they definitely need a ton of improvements, that's why I didn't want to commit to create and apply real database schema

JOB_TYPE_SRPM = "SRPM"
JOB_TYPE_COPR_RPM = "COPR-RPM"
JOB_TYPE_TFT = "TFT"
Comment on lines +195 to +199

This comment has been minimized.

Copy link
@lachmanfrantisek

lachmanfrantisek Jan 10, 2020

Member

What about some enum -- will be probably useful elsewhere.

This comment has been minimized.

Copy link
@TomasTomecek

TomasTomecek Jan 10, 2020

Author Contributor

as for enums:

  • didn't dig into how database engines or SA deal with enums
  • I hate when enums are represented as numbers in DB - what the F should know what 2 or 3 does stand for?
  • I hope it wouldn't be a big problem to alter the enum in future
  • actually I think I hate enums, they bring more problems than benefits

how about I just add your suggestion as a comment to the code?

This comment has been minimized.

Copy link
@lachmanfrantisek

lachmanfrantisek Jan 10, 2020

Member
* I hate when enums are represented as numbers in DB - what the F should know what 2 or 3 does stand for?

You can use a string enum on the top of this. (So the values are strings, not ints.)

how about I just add your suggestion as a comment to the code?

👍

@TomasTomecek TomasTomecek force-pushed the TomasTomecek:use-pg branch from 361f029 to ac5c019 Jan 10, 2020
@TomasTomecek

This comment has been minimized.

Copy link
Contributor Author

TomasTomecek commented Jan 10, 2020

Thanks for reviews, guys.

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Jan 10, 2020

Build failed.

@TomasTomecek TomasTomecek removed the mergeit label Jan 13, 2020
@jpopelka

This comment has been minimized.

Copy link
Member

jpopelka commented Jan 30, 2020

So what's the state? (other than needs-rebase)

@TomasTomecek

This comment has been minimized.

Copy link
Contributor Author

TomasTomecek commented Feb 6, 2020

needs-free-time

@TomasTomecek TomasTomecek force-pushed the TomasTomecek:use-pg branch from ac5c019 to 6c01dd0 Feb 6, 2020
@TomasTomecek

This comment has been minimized.

Copy link
Contributor Author

TomasTomecek commented Feb 6, 2020

rebased, let's see

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Feb 6, 2020

Build failed.

@TomasTomecek TomasTomecek force-pushed the TomasTomecek:use-pg branch from 6c01dd0 to 628c48e Feb 10, 2020
@TomasTomecek

This comment has been minimized.

Copy link
Contributor Author

TomasTomecek commented Feb 10, 2020

this was the last rebase

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Feb 10, 2020

Build failed.

@TomasTomecek TomasTomecek removed the mergeit label Feb 12, 2020
TomasTomecek added 2 commits Jan 9, 2020
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
@TomasTomecek TomasTomecek force-pushed the TomasTomecek:use-pg branch from 628c48e to 23a12a5 Feb 12, 2020
@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Feb 12, 2020

Build succeeded.

@softwarefactory-project-zuul

This comment has been minimized.

Copy link
Contributor

softwarefactory-project-zuul bot commented Feb 12, 2020

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 6adea3c into packit-service:master Feb 12, 2020
3 checks passed
3 checks passed
LGTM analysis: Python No new or fixed alerts
Details
local/check check status: success
Details
local/gate gate status: success
Details
@TomasTomecek TomasTomecek deleted the TomasTomecek:use-pg branch Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.