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 2020/08/14 23:58:20 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #10333: Unify error messages and complete type field in response
ephraimbuddy opened a new pull request #10333:
URL: https://github.com/apache/airflow/pull/10333
Closes: #9736
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] OmairK commented on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
OmairK commented on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-678365011
@ephraimbuddy can you rebase to the latest master, #9740 is merged can you update the error message for `patch_dag` endpoint, thanks in advance. :smile_cat:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#discussion_r479612607
##########
File path: airflow/api_connexion/exceptions.py
##########
@@ -16,48 +16,134 @@
# under the License.
from typing import Dict, Optional
-from connexion import ProblemException
+import werkzeug
+from connexion import FlaskApi, ProblemException, problem
+
+from airflow import version
+
+doc_link = f'https://airflow.apache.org/docs/{version.version}/stable-rest-api-ref.html'
+if 'dev' in version.version:
+ doc_link = "https://airflow.readthedocs.io/en/latest/stable-rest-api-ref.html"
+
+EXCEPTIONS_LINK_MAP = {
+ 400: doc_link + "#section/Errors/BadRequest",
+ 404: doc_link + "#section/Errors/NotFound",
+ 401: doc_link + "#section/Errors/Unauthenticated",
+ 409: doc_link + "#section/Errors/AlreadyExists",
+ 403: doc_link + "#section/Errors/PermissionDenied",
+ 500: doc_link + "#section/Errors/Unknown",
Review comment:
nitpick, it would be better to use f string here as well to be consistent with the rest of the code base. f string is also slightly faster than manual string cocnat.
##########
File path: airflow/api_connexion/exceptions.py
##########
@@ -16,48 +16,134 @@
# under the License.
from typing import Dict, Optional
-from connexion import ProblemException
+import werkzeug
+from connexion import FlaskApi, ProblemException, problem
+
+from airflow import version
+
+doc_link = f'https://airflow.apache.org/docs/{version.version}/stable-rest-api-ref.html'
+if 'dev' in version.version:
+ doc_link = "https://airflow.readthedocs.io/en/latest/stable-rest-api-ref.html"
+
+EXCEPTIONS_LINK_MAP = {
+ 400: doc_link + "#section/Errors/BadRequest",
+ 404: doc_link + "#section/Errors/NotFound",
+ 401: doc_link + "#section/Errors/Unauthenticated",
+ 409: doc_link + "#section/Errors/AlreadyExists",
+ 403: doc_link + "#section/Errors/PermissionDenied",
+ 500: doc_link + "#section/Errors/Unknown",
+}
+
+
+def common_error_handler(exception):
+ """
+ Used to capture connexion exceptions and add link to the type field
+ :type exception: Exception
+ """
+ if isinstance(exception, ProblemException):
+
+ if EXCEPTIONS_LINK_MAP.get(exception.status, None):
+ response = problem(
+ status=exception.status,
+ title=exception.title,
+ detail=exception.detail,
+ type=EXCEPTIONS_LINK_MAP[exception.status],
+ instance=exception.instance,
+ headers=exception.headers,
+ ext=exception.ext,
+ )
Review comment:
nitpick, the subsequent map lookup can be avoided if we reuse the result from the first get call.
```suggestion
link = EXCEPTIONS_LINK_MAP.get(exception.status)
if link:
response = problem(
status=exception.status,
title=exception.title,
detail=exception.detail,
type=link,
instance=exception.instance,
headers=exception.headers,
ext=exception.ext,
)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-680286814
@houqp Can I aak for a review? If I remember correctly, you had a question how do we want to implement it. Does it meet your expectations?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy edited a comment on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
ephraimbuddy edited a comment on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-677909245
I can't figure out why I can't run breeze on this branch🙂, will really appreciate your helps cc: @potiuk , @mik-laj
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj merged pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #10333:
URL: https://github.com/apache/airflow/pull/10333
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-680279942
Can you rebase on latest master please @ephraimbuddy
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#discussion_r473559486
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -19,7 +19,29 @@ openapi: 3.0.3
info:
title: "Airflow API (Stable)"
- description: Apache Airflow management API.
+ description: |
+ Apache Airflow management API.
+
+ # Errors
+
+ We follow the error response format proposed in [RFC 7807](https://tools.ietf.org/html/rfc7807)
+ also known as Problem Details for HTTP APIs. As with our normal API responses,
+ your client must be prepared to gracefully handle additional members of the response.
+ ## Unauthenticated
+ <RedocResponse pointer={"#/components/responses/Unauthenticated"} />
Review comment:
This element is not supported in the Open Source Redoc version. I would just write 2-3 sentences here what this error message means and when it happens and what can be done to fix it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-682752638
Can you squash al commits and do a rebase?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-678475229
I don't really know what caused the earlier breeze issues🤨. I had to delete the branch and recreate it at an earlier commit. Then after rebasing, it worked. However, I force pushed, using only `--force`😠, discarding 2 commits that was already here. I'm really sorry for any issue this might cause anyone😔.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-682752638
Can you quash al commits and do a rebase?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#discussion_r475155102
##########
File path: airflow/api_connexion/exceptions.py
##########
@@ -16,45 +16,141 @@
# under the License.
from typing import Dict, Optional
-from connexion import ProblemException
+import werkzeug
+from connexion import FlaskApi, ProblemException, problem
+
+from airflow import version
+
+doc_link = f'https://airflow.apache.org/docs/{version.version}/stable-rest-api/redoc.html'
+if 'dev' in version.version:
+ doc_link = "https://airflow.readthedocs.io/en/latest/stable-rest-api/redoc.html"
Review comment:
```suggestion
doc_link = "https://airflow.readthedocs.io/en/latest/stable-rest-api-ref.html"
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-680285503
Alright🚀
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-677909245
I can't figure out why I can't run breeze on this branch, will really appreciate your helps cc: @potiuk , @mik-laj
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy removed a comment on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
ephraimbuddy removed a comment on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-678475229
I don't really know what caused the earlier breeze issues🤨. I had to delete the branch and recreate it at an earlier commit. Then after rebasing, it worked. However, I force pushed, using only `--force`😠, discarding 2 commits that was already here. I'm really sorry for any issue this might cause anyone😔.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-678372479
Nice🚀, I will do that.🙂
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #10333: Unify error messages and complete type field in response
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #10333:
URL: https://github.com/apache/airflow/pull/10333#issuecomment-680286814
@houqp Can I ask for a review? If I remember correctly, you had a question how do we want to implement it. Does it meet your expectations?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org