Skip to content

Conversation

@o-nikolas
Copy link
Contributor

Part of a project to add, simplify and standardize AWS sample dags and docs in preparation for adding System Testing.

related: #21828
related: #22010
related: #21920


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 15, 2022
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

Comment on lines 94 to 103
Copy link
Contributor

Choose a reason for hiding this comment

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

We can have less code lines with the python decorator

Copy link
Contributor

Choose a reason for hiding this comment

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

We really should be using the TaskFlow API where possible. There was an effort a while back to transition the example DAGs to this approach. Related to #9415

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, folks, I will make the change :)

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

As @josh-fell mentioned - using @task is much better than Python Operators :)

@o-nikolas o-nikolas force-pushed the onikolas/glue-docs-dags branch from 1ffdcd6 to ca0b916 Compare March 18, 2022 23:20
Copy link
Contributor

@josh-fell josh-fell left a comment

Choose a reason for hiding this comment

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

Looks awesome! Just a small TaskFlow API update to use the .output property of operators.

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
run_id="{{ ti.xcom_pull(task_ids='submit_glue_job') }}",
run_id=submit_glue_job.output,

Another perk of the TaskFlow API: using the .output property or XComArgs. This is a functional abstraction over the classic "{{ ti.xcom_pull(...) }}" approach to pull XComs from operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated the PR with this suggestion :)

Note: I also removed the header from the example CSV file since it was causing issues in the glue job, so when you see that in the diff, it was intentional 👍

@o-nikolas o-nikolas force-pushed the onikolas/glue-docs-dags branch from ca0b916 to 17116bb Compare March 21, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers kind:documentation okay to merge It's ok to merge this PR as it does not require more tests provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants