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

add timeout to influx db #40439

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Conversation

romsharon98
Copy link
Contributor

@romsharon98 romsharon98 commented Jun 26, 2024

passing extra to influxDB.
close this thread in slack:


^ 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 Jun 26, 2024

Hmm. I guess we should add (or modify) existing test to test for it. I find it highly suspicious when such changes pass without any test modifications - it makes me think some tests are missing.

``token``: (required) `Create token <https://docs.influxdata.com/influxdb/cloud/security/tokens/create-token/>`_
using the influxdb cli or UI

``org_name``: (required) `Create org <https://docs.influxdata.com/influxdb/cloud/reference/cli/influx/org/create/>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

If token and org name are required lets customize the Ui form for to have them. Its much nicer than passing them as kwargs

Copy link
Contributor

Choose a reason for hiding this comment

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

@romsharon98 i think we shouldnt remove this but just mention its not defined via extra.
This is what we do in other providers as well
https://airflow.apache.org/docs/apache-airflow-providers-google/stable/connections/bigquery.html#configuring-the-connection

@romsharon98
Copy link
Contributor Author

Hmm. I guess we should add (or modify) existing test to test for it. I find it highly suspicious when such changes pass without any test modifications - it makes me think some tests are missing.

Yes, you were right. The tests didn't check much (or at all). I've updated them.

@romsharon98 romsharon98 self-assigned this Jun 27, 2024
@romsharon98 romsharon98 requested a review from eladkal June 27, 2024 06:52
@potiuk potiuk merged commit e4c125f into apache:main Jun 27, 2024
51 checks passed
@potiuk
Copy link
Member

potiuk commented Jun 27, 2024

Cool. Thanks!

potiuk added a commit to potiuk/airflow that referenced this pull request Jun 27, 2024
The apache#40439 introduced new attributes for influxdb widgets, with
the InputRequired validator, which should not be used on connection
fields, because the fields are all present in the form for all
connection types, and they are just "hidden" dynamically per
connection type. This PR removes InputRequired validation for those
potiuk added a commit that referenced this pull request Jun 27, 2024
The #40439 introduced new attributes for influxdb widgets, with
the InputRequired validator, which should not be used on connection
fields, because the fields are all present in the form for all
connection types, and they are just "hidden" dynamically per
connection type. This PR removes InputRequired validation for those
romsharon98 added a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
…#40469)

The apache#40439 introduced new attributes for influxdb widgets, with
the InputRequired validator, which should not be used on connection
fields, because the fields are all present in the form for all
connection types, and they are just "hidden" dynamically per
connection type. This PR removes InputRequired validation for those
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