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

Validate SqoopHook connection string and disable extra options from public hook methods #33039

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

pankajkoti
Copy link
Member

@pankajkoti pankajkoti commented Aug 2, 2023

Check that the connection string constructed using the connection's
host, port and schema does not contain query params as it is
not intended. Additionally, also disable the extra_import_options
and extra_export_options arguments accepted directly by the hook
methods but accept it as a param via the hook constructor when
initialising the hook or by passing it in hook_params when initialising
the hook from operators.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member

potiuk commented Aug 2, 2023

Pending tests passing :)

@pankajkoti pankajkoti marked this pull request as draft August 2, 2023 15:10
@pankajkoti pankajkoti marked this pull request as ready for review August 3, 2023 06:38
@pankajkoti
Copy link
Member Author

PR is ready for review
cc: @potiuk @eladkal

@pankajkoti pankajkoti requested a review from eladkal August 3, 2023 09:32
…ook methods

Check that the connection string constructed using the connection's
`host`, `port` and `schema` does not contain query params as it is not
intended. Additionally, also disable the `extra_import_options` and
`extra_export_options` arguments accepted directly by the hook
methods but accept it as a param via the hook constructor when
initialising the hook or by passing it in hook_params when initialising
the hook from operators.
@pankajkoti
Copy link
Member Author

Test constraints check seems to be timing out repeatedly after 80 minutes.

@potiuk
Copy link
Member

potiuk commented Aug 3, 2023

Test constraints check seems to be timing out repeatedly after 80 minutes.

Yes. Seems there is a backtracking of pip in main (I.e. it has difficulties in solving recent constraints) after some dependency release. will take a look.

@potiuk
Copy link
Member

potiuk commented Aug 3, 2023

BTW. I just realised that PR's like this should not cause the "--upgrade-to-newer-dependencies` (this is what triggers the backtracking issue) fix to CI infrastructure is coming

@potiuk
Copy link
Member

potiuk commented Aug 3, 2023

After this one is merged : #33082 and after your rebase, the constraint issue should go away even if we do not fix it in main.

@potiuk
Copy link
Member

potiuk commented Aug 3, 2023

Merging, regardless.

@potiuk potiuk merged commit 59f5f58 into apache:main Aug 3, 2023
41 of 42 checks passed
ephraimbuddy pushed a commit that referenced this pull request Aug 28, 2023
…ublic hook methods (#33039)

* Validate SqoopHook connection string and disable extra options from hook methods

Check that the connection string constructed using the connection's
`host`, `port` and `schema` does not contain query params as it is not
intended. Additionally, also disable the `extra_import_options` and
`extra_export_options` arguments accepted directly by the hook
methods but accept it as a param via the hook constructor when
initialising the hook or by passing it in hook_params when initialising
the hook from operators.

* Propogate missing hook param changes to the operator

* Remove test for invalid port with query param as ports are integers and not accepted by databases

* Add 4.0.0 to provider.yaml

(cherry picked from commit 59f5f58)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants