You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/07/27 17:39:37 UTC
[GitHub] [airflow] josh-fell opened a new pull request #17269: Handle connection parameters added to Extra and custom fields
josh-fell opened a new pull request #17269:
URL: https://github.com/apache/airflow/pull/17269
Closes: #17235
Adding logic to handle connection parameters added to the classic `Extra` fields as well as part of custom fields which use `Extra` under the hood. Currently if parameters are added to the classic `Extra` field, they are overwritten by values in custom fields.
This PR will combine "extras" from both types, if applicable, so existing hooks can still use the `Extra` field within connections if needed.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17269: Handle connection parameters added to Extra and custom fields
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17269:
URL: https://github.com/apache/airflow/pull/17269#discussion_r677977379
##########
File path: airflow/www/views.py
##########
@@ -3226,6 +3226,12 @@ def process_form(self, form, is_created):
for key in self.extra_fields
if key in form.data and key.startswith(f"extra__{conn_type}__")
}
+
+ # If parameters are added to the classic `Extra` field, include these values along with
+ # custom-field extras.
+ if form.data.get("extra"):
+ extra.update(json.loads(form.data.get("extra")))
Review comment:
The test cases should also cover these “valid in form but invalid in logic” values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17269: Handle connection parameters added to Extra and custom fields
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17269:
URL: https://github.com/apache/airflow/pull/17269#discussion_r678786577
##########
File path: airflow/www/views.py
##########
@@ -3226,6 +3226,12 @@ def process_form(self, form, is_created):
for key in self.extra_fields
if key in form.data and key.startswith(f"extra__{conn_type}__")
}
+
+ # If parameters are added to the classic `Extra` field, include these values along with
+ # custom-field extras.
+ if form.data.get("extra"):
+ extra.update(json.loads(form.data.get("extra")))
Review comment:
I think this should fail hard on the user as well, but IIUC this would result in a 500 page, which doesn’t feel right. Dropping the extra value + a flash sounds much better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #17269: Handle connection parameters added to Extra and custom fields
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #17269:
URL: https://github.com/apache/airflow/pull/17269#discussion_r678351223
##########
File path: airflow/www/views.py
##########
@@ -3226,6 +3226,12 @@ def process_form(self, form, is_created):
for key in self.extra_fields
if key in form.data and key.startswith(f"extra__{conn_type}__")
}
+
+ # If parameters are added to the classic `Extra` field, include these values along with
+ # custom-field extras.
+ if form.data.get("extra"):
+ extra.update(json.loads(form.data.get("extra")))
Review comment:
Are you thinking this should not fail if an invalid dictionary value is added? I assumed this would fail hard and the user would need to recreate the connection again. Happy to flash an error message but still create the connection entry or catch some exceptions and log a custom failure message for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #17269: Handle connection parameters added to Extra and custom fields
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17269:
URL: https://github.com/apache/airflow/pull/17269#issuecomment-889647566
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #17269: Handle connection parameters added to Extra and custom fields
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #17269:
URL: https://github.com/apache/airflow/pull/17269#discussion_r678351223
##########
File path: airflow/www/views.py
##########
@@ -3226,6 +3226,12 @@ def process_form(self, form, is_created):
for key in self.extra_fields
if key in form.data and key.startswith(f"extra__{conn_type}__")
}
+
+ # If parameters are added to the classic `Extra` field, include these values along with
+ # custom-field extras.
+ if form.data.get("extra"):
+ extra.update(json.loads(form.data.get("extra")))
Review comment:
Are you thinking this should not fail if an invalid dictionary value is added? I assumed this would fail hard and the user would need to recreate the connection entry. Happy to flash an error message but still create the connection entry or catch some exceptions and log a custom failure message for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a change in pull request #17269: Handle connection parameters added to Extra and custom fields
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #17269:
URL: https://github.com/apache/airflow/pull/17269#discussion_r678780369
##########
File path: airflow/www/views.py
##########
@@ -3226,6 +3226,12 @@ def process_form(self, form, is_created):
for key in self.extra_fields
if key in form.data and key.startswith(f"extra__{conn_type}__")
}
+
+ # If parameters are added to the classic `Extra` field, include these values along with
+ # custom-field extras.
+ if form.data.get("extra"):
+ extra.update(json.loads(form.data.get("extra")))
Review comment:
Perhaps it is a better UX for the connection entry to be created but flash an error message that the invalid `Extra` value(s) are not added. Something like this:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17269: Handle connection parameters added to Extra and custom fields
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17269:
URL: https://github.com/apache/airflow/pull/17269#discussion_r677977131
##########
File path: airflow/www/views.py
##########
@@ -3226,6 +3226,12 @@ def process_form(self, form, is_created):
for key in self.extra_fields
if key in form.data and key.startswith(f"extra__{conn_type}__")
}
+
+ # If parameters are added to the classic `Extra` field, include these values along with
+ # custom-field extras.
+ if form.data.get("extra"):
+ extra.update(json.loads(form.data.get("extra")))
Review comment:
This would break if `extra` is something like `"null"` or `1`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #17269: Handle connection parameters added to Extra and custom fields
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #17269:
URL: https://github.com/apache/airflow/pull/17269
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org