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

modularization: Worksheets modularization #449

Merged

Conversation

dbalintx
Copy link
Contributor

@dbalintx dbalintx commented May 10, 2023

Modularization of Worksheets

  • Moved Worksheet related code to its own new module
  • Merged AthenaQueryResult object into Worksheets
  • Worksheet related permissions moved to new module
  • Removed worksheet sharing related (unused) code from the entire repo

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dbalintx dbalintx requested review from dlpzx and nikpodsh May 10, 2023 18:32
@nikpodsh
Copy link
Contributor

We can also move/remove WorksheetRole from db.models.WorksheetRole

@dbalintx
Copy link
Contributor Author

@dlpzx @nikpodsh I addressed the review comments, could you please double-check the content and approve/provide feedback? :) Ty!

@@ -1253,18 +1253,6 @@ def upgrade():
sa.Column('created', sa.DateTime(), nullable=True),
sa.PrimaryKeyConstraint('AthenaQueryId'),
)
op.create_table(
Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot modify pre-existing alembic migration scripts. If we are going to delete a table we need to create a new script deleting it otherwise nothing gets deleted in the pre-existing deployments

Copy link
Contributor

Choose a reason for hiding this comment

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

In a new deployment, all migration scripts are run in order, so the changes will work. But let's say we have a deployment in V1.3, when we upgrade the CodeBuild dbmigration stage runs alembic upgrade (or something similar) and executes the new migration scripts only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we discussed, I removed the unnecessary changes from the former migration scripts, and fixed the changes in the latest one

Copy link
Contributor

@dlpzx dlpzx May 16, 2023

Choose a reason for hiding this comment

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

Looks good, I will try to test the migration scripts, otherwise I think it is good

Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

We need to modify the migration scripts before merging

@dlpzx dlpzx added this to In progress in v2.0.0 Code Modularization via automation May 17, 2023
@dlpzx dlpzx added the type: modularization Code refactoring project label May 17, 2023
@dlpzx dlpzx moved this from In progress to Review in progress in v2.0.0 Code Modularization May 17, 2023
@nikpodsh nikpodsh merged commit 45c1b33 into data-dot-all:modularization-main May 17, 2023
v2.0.0 Code Modularization automation moved this from Review in progress to Done May 17, 2023
@dlpzx dlpzx changed the title Worksheets modularization modularization: Worksheets modularization May 24, 2023
dlpzx pushed a commit to dlpzx/aws-dataall that referenced this pull request May 25, 2023
Modularization of Worksheets

- Moved Worksheet related code to its own new module
- Merged AthenaQueryResult object into Worksheets
- Worksheet related permissions moved to new module
- Removed worksheet sharing related (unused) code from the entire repo

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: modularization Code refactoring project
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants