Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[DEV-6356] added table installation_progress #3547
Changes from 3 commits
b992d89
6955cab
dea89ff
6fa6add
0da39ac
48d6158
0b55fdf
5634945
3f00fdd
375f12d
d1ffa7b
5abad5a
1356611
a49ced2
7093240
4ae7d01
c756fe4
1768fbe
22afbc6
9e76d64
0e722cc
0f0959c
6dd8683
c75df89
9eb9962
dd1397c
f447930
6b4c157
2ab0a34
82ba63f
613557c
fedac7e
f223233
4e9aa26
acc5dff
23ed10a
30fbb69
117ec37
de9e9fe
fbac3b0
f26d5e9
69be81d
a4b2d31
a0ab1ce
ca798c4
4e5711e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
Also, note that
primary_key=True
impliesnullable=False
under the hood.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok removed
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.