Skip to content

Conversation

@piffall
Copy link
Contributor

@piffall piffall commented Feb 2, 2021

Adding Connections parameter to AwsGlueJobHook in order to previse which subnet must be used for Job deployment.

If we have a Connection only accessible from an specific subnet (not the default one) and we do not pass this parameter, Job will be deployed to default subnet, so will not have access to DDBB, resulting in a Timeout error.

This small patch fix this issue based on Glue Client documentation.

^ 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.

@boring-cyborg boring-cyborg bot added the provider:amazon AWS/Amazon - related issues label Feb 2, 2021
@github-actions
Copy link

github-actions bot commented Feb 3, 2021

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Feb 3, 2021

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@piffall piffall force-pushed the master branch 3 times, most recently from a6931a5 to 352e60a Compare February 3, 2021 11:59
@eladkal eladkal requested a review from feluelle February 4, 2021 16:07
@github-actions
Copy link

github-actions bot commented Feb 5, 2021

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@piffall
Copy link
Contributor Author

piffall commented Feb 5, 2021

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

Hi again @dstandish, there was a weird error on build. It seems that had failed on Documentation style, but there are not any change on the PR. Could you check it out? How would I proceed?

Thanks!

@dstandish
Copy link
Contributor

@piffall
Copy link
Contributor Author

piffall commented Feb 8, 2021

you have a pre-commit hook failing:
https://github.com/apache/airflow/pull/14027/checks?check_run_id=1838235536#step:7:123

Hi, @dstandish . I've passed pre-commit run --all-files locally without errors. Is the CI pipeline merging code before running tests?

@dstandish
Copy link
Contributor

oh you might just need to rebase this was an issue briefly in master i think

@piffall
Copy link
Contributor Author

piffall commented Feb 8, 2021

oh you might just need to rebase this was an issue briefly in master i think

Hi @dstandish , It seems to be resolved after rebase. Thanks! ;)

@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 Feb 26, 2021
@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 master or amend the last commit of the PR, and push it with --force-with-lease.

@feluelle feluelle merged commit 13854c3 into apache:master Feb 27, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 27, 2021

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants