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

[DEV-6356] added table installation_progress #3547

Closed
wants to merge 46 commits into from

Conversation

Ildyakov
Copy link

@Ildyakov Ildyakov commented Apr 4, 2023

This change is Reviewable

@Ildyakov Ildyakov requested a review from lwsanty April 4, 2023 18:48
@Ildyakov
Copy link
Author

Ildyakov commented Apr 4, 2023

@lwsanty lwsanty requested a review from dennwc April 4, 2023 18:52
),
sa.Column(
"account_created",
sa.TIMESTAMP(timezone=True),
Copy link
Member

Choose a reason for hiding this comment

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

can we maybe already populate this column for existing accounts? or we don't have to?
also, do we need to fill it during the account creation in accounts table?
cc @dennwc

Copy link
Author

Choose a reason for hiding this comment

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

probably we don`t need an existing accounts at all. the point of this feature is just for tracking new installations

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Olek: we should create records for existing accounts. This is where the current_status will help - we can set it, but leave timestamps empty. Maybe we can update the table later to reflect actual installations that happened in the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we usually do this manually with a data post-migration. It's worth setting a separate task to backfill the history.

Comment on lines 22 to 59
sa.Column(
"account_id",
sa.Integer(),
sa.ForeignKey("accounts.id", name="fk_installation_progress_account"),
primary_key=True,
),
sa.Column(
"account_created",
sa.TIMESTAMP(timezone=True),
),
sa.Column(
"fetch_started",
sa.TIMESTAMP(timezone=True),
),
sa.Column(
"fetch_completed",
sa.TIMESTAMP(timezone=True),
),
sa.Column(
"consistency_started",
sa.TIMESTAMP(timezone=True),
),
sa.Column(
"consistency_completed",
sa.TIMESTAMP(timezone=True),
),
sa.Column(
"precompute_started",
sa.TIMESTAMP(timezone=True),
),
sa.Column(
"precompute_completed",
sa.TIMESTAMP(timezone=True),
),
sa.Column(
"current_status",
sa.Text(),
),
Copy link
Member

Choose a reason for hiding this comment

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

can all these timestamps be null?

Copy link
Author

Choose a reason for hiding this comment

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

yep and they will be

server/athenian/api/models/state/models.py Outdated Show resolved Hide resolved

account_id = Column(
Integer(),
ForeignKey("accounts.id", name="fk_installation_progress_account"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This FK assumes that there must be an Athenian account associated with the record. This is not the case for GitHub installations: they usually start without Athenian account. So we won't be able to insert most of the records here, unless we force Athenian account creation from MD side.

Copy link
Author

Choose a reason for hiding this comment

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

ok removed

Copy link
Contributor

Choose a reason for hiding this comment

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

So what's your plan for the account_id column? Is it still an Athenian account ID or GitHub account ID?

Copy link
Author

Choose a reason for hiding this comment

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

this have to be a Github ID as it is in the Event

),
sa.Column(
"account_created",
sa.TIMESTAMP(timezone=True),
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Olek: we should create records for existing accounts. This is where the current_status will help - we can set it, but leave timestamps empty. Maybe we can update the table later to reflect actual installations that happened in the past.


__tablename__ = "installation_progress"

account_id = Column(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please format every field in one line for better readability and consistency with the surrounding models 🙏 E.g.

Suggested change
account_id = Column(
account_id = Column(Integer(), primary_key=True)

Also, note that primary_key=True implies nullable=False under the hood.

primary_key=True,
)
account_created = Column(
"account_created",
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have to set the name for each column, luckily. This is enough:

account_created = Column(TIMESTAMP(timezone=True))

sa.Column(
"account_id",
sa.Integer(),
nullable=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
nullable=False,

),
sa.Column(
"account_created",
sa.TIMESTAMP(timezone=True),
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we usually do this manually with a data post-migration. It's worth setting a separate task to backfill the history.

gkwillie and others added 23 commits April 11, 2023 10:40
Bumps [morcilla[postgresql,sqlite]](https://github.com/athenianco/morcilla) from 0.5.35 to 0.5.37.
- [Release notes](https://github.com/athenianco/morcilla/releases)
- [Commits](athenianco/morcilla@v0.5.35...v0.5.37)

---
updated-dependencies:
- dependency-name: morcilla[postgresql,sqlite]
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [sentry-sdk[pure_eval]](https://github.com/getsentry/sentry-python) from 1.17.0 to 1.19.1.
- [Release notes](https://github.com/getsentry/sentry-python/releases)
- [Changelog](https://github.com/getsentry/sentry-python/blob/master/CHANGELOG.md)
- [Commits](getsentry/sentry-python@1.17.0...1.19.1)

---
updated-dependencies:
- dependency-name: sentry-sdk[pure_eval]
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [gcloud-aio-auth](https://github.com/talkiq/gcloud-aio) from 4.2.0 to 4.2.1.
- [Release notes](https://github.com/talkiq/gcloud-aio/releases)
- [Commits](talkiq/gcloud-aio@kms-4.2.0...auth-4.2.1)

---
updated-dependencies:
- dependency-name: gcloud-aio-auth
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [gcloud-aio-pubsub](https://github.com/talkiq/gcloud-aio) from 5.3.0 to 5.4.0.
- [Release notes](https://github.com/talkiq/gcloud-aio/releases)
- [Commits](talkiq/gcloud-aio@pubsub-5.3.0...pubsub-5.4.0)

---
updated-dependencies:
- dependency-name: gcloud-aio-pubsub
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [types-python-dateutil](https://github.com/python/typeshed) from 2.8.19.11 to 2.8.19.12.
- [Release notes](https://github.com/python/typeshed/releases)
- [Commits](https://github.com/python/typeshed/commits)

---
updated-dependencies:
- dependency-name: types-python-dateutil
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [coverage](https://github.com/nedbat/coveragepy) from 7.2.2 to 7.2.3.
- [Release notes](https://github.com/nedbat/coveragepy/releases)
- [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst)
- [Commits](nedbat/coveragepy@7.2.2...7.2.3)

---
updated-dependencies:
- dependency-name: coverage
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [faker](https://github.com/joke2k/faker) from 18.3.1 to 18.4.0.
- [Release notes](https://github.com/joke2k/faker/releases)
- [Changelog](https://github.com/joke2k/faker/blob/master/CHANGELOG.md)
- [Commits](joke2k/faker@v18.3.1...v18.4.0)

---
updated-dependencies:
- dependency-name: faker
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [pytest](https://github.com/pytest-dev/pytest) from 7.2.2 to 7.3.0.
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@7.2.2...7.3.0)

---
updated-dependencies:
- dependency-name: pytest
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@Ildyakov Ildyakov requested a review from a team as a code owner April 11, 2023 15:40
@vmarkovtsev
Copy link
Contributor

vmarkovtsev commented Apr 12, 2023

@Ildyakov merge conflicts, let's rebase

@vmarkovtsev
Copy link
Contributor

I think that there is a new clean PR: #3564

@vmarkovtsev vmarkovtsev deleted the DEV-6356-migration-installation-table-create branch April 13, 2023 09:53
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.

5 participants