-
Notifications
You must be signed in to change notification settings - Fork 18
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
Adds workflow_dag engine_config column #209
Adds workflow_dag engine_config column #209
Conversation
d90961a
to
15f5939
Compare
c985837
to
f1ed136
Compare
6345003
to
ef78cd5
Compare
3ca8c68
to
92114b5
Compare
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.
The test plan looks very solid!
src/golang/cmd/migrator/versions/000013_add_workflow_dag_engine_config/up_postgres.go
Show resolved
Hide resolved
@@ -29,7 +29,7 @@ import ( | |||
) | |||
|
|||
const ( | |||
RequiredSchemaVersion = 8 |
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.
wow where is this field used? I'm surprised that nothing has been broken so far
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.
Well this was more important in the enterprise project. Here once we modify the install_local
and packaging scripts, we know that the next release will be running the correct schema version.
We do have a check here https://github.com/aqueducthq/aqueduct/blob/main/src/golang/cmd/server/server/aqueduct_server.go#L118 just to confirm.
92114b5
to
de41446
Compare
Describe your changes and why you are making these changes
This PR adds an
engine_config
column to theworkflow_dag
table. This is necessary to store important information for mapping an Aqueduct workflow to the native concepts of the 3rd-party engine the workflow is set to run on, such as Airflow.Related issue number (if any)
ENG-1243
Checklist before requesting a review
python3 scripts/run_linters.py -h
for usage).run_integration_test
: Runs integration testsskip_integration_test
: Skips integration tests (Should be used when changes are ONLY documentation/UI)Test
I tested this schema change by creating a workflow (and workflow_dag) before applying the schema change. Then, I applied the schema change and confirmed that the new column was created. After this, I tested that the
WorkflowDagReader
was able to successfully read theengine_config
column.