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/16 15:06:10 UTC
[GitHub] [airflow] jpmarques66 opened a new pull request #10351: Migrate speccy to spectral in OpenAPI linting.
jpmarques66 opened a new pull request #10351:
URL: https://github.com/apache/airflow/pull/10351
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Replaces speccy with spectral for OpenAPI linting.
Fixes a bug where sibling properties of `$ref` were being ignored, as describe in the [OpenAPI docs](https://swagger.io/docs/specification/using-ref/).
Closes #10001
@mik-laj Let me know if there's anything else you need me to do regarding this 😃 .
---
**^ 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] houqp commented on a change in pull request #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#discussion_r481866791
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1447,8 +1447,9 @@ components:
readOnly: true
nullable: true
state:
- $ref: '#/components/schemas/DagState'
readOnly: True
+ oneOf:
+ - $ref: '#/components/schemas/DagState'
Review comment:
Based on the docs you linked, i think the change you introduced looks correct, but I agree this is better handled as a separate follow issue.
----------------------------------------------------------------
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 pull request #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#issuecomment-686806300
I think it's worth digging into the sibling issue as follow up. If it's a bug in openapi-generator, we should fix it from there, or maybe we can figure out a better way to mark objects as readonly.
----------------------------------------------------------------
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] jpmarques66 commented on pull request #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
jpmarques66 commented on pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#issuecomment-684917287
@mik-laj @houqp I reverted the `v1.yaml` modifications that impacted the go code generation. I realized this was out of scope.
----------------------------------------------------------------
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 #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #10351:
URL: https://github.com/apache/airflow/pull/10351
----------------------------------------------------------------
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] jpmarques66 commented on pull request #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
jpmarques66 commented on pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#issuecomment-683292166
@mik-laj Fixed CI.
----------------------------------------------------------------
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] jpmarques66 commented on a change in pull request #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
jpmarques66 commented on a change in pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#discussion_r481175895
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1447,8 +1447,9 @@ components:
readOnly: true
nullable: true
state:
- $ref: '#/components/schemas/DagState'
readOnly: True
+ oneOf:
+ - $ref: '#/components/schemas/DagState'
Review comment:
So that was the idea in the beginning since spectral started complaining about that (having siblings of $ref). The [docs](https://swagger.io/docs/specification/using-ref/) refer that siblings are ignored, but it seems that the code generation isn't prepared for this.
I'm still a bit unfamiliar with the project, do you think this should be reverted?
----------------------------------------------------------------
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 pull request #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#issuecomment-684552078
based on the diff, it does seem to cause openapi-generator to generate less sensible code:
```diff
diff -u ./clients/go_target_branch/airflow/model_dag_run.go ./clients/go/airflow/model_dag_run.go
--- ./clients/go_target_branch/airflow/model_dag_run.go 2020-08-29 20:35:34.312964227 +0000
+++ ./clients/go/airflow/model_dag_run.go 2020-08-29 20:35:30.524903466 +0000
@@ -22,7 +22,7 @@
// The start time. The time when DAG run was actually created.
StartDate time.Time `json:"start_date,omitempty"`
EndDate *time.Time `json:"end_date,omitempty"`
- State DagState `json:"state,omitempty"`
+ State OneOfDagState `json:"state,omitempty"`
ExternalTrigger bool `json:"external_trigger,omitempty"`
// JSON object describing additional configuration parameters. The value of this field can be set only when creating the object. If you try to modify the field of an existing object, the request fails with an BAD_REQUEST error.
Conf map[string]interface{} `json:"conf,omitempty"`
diff -u ./clients/go_target_branch/airflow/model_task.go ./clients/go/airflow/model_task.go
--- ./clients/go_target_branch/airflow/model_task.go 2020-08-29 20:35:34.476966679 +0000
+++ ./clients/go/airflow/model_task.go 2020-08-29 20:35:30.668906920 +0000
@@ -27,8 +27,8 @@
Queue string `json:"queue,omitempty"`
Pool string `json:"pool,omitempty"`
PoolSlots float32 `json:"pool_slots,omitempty"`
- ExecutionTimeout TimeDelta `json:"execution_timeout,omitempty"`
- RetryDelay TimeDelta `json:"retry_delay,omitempty"`
+ ExecutionTimeout OneOfnullTimeDelta `json:"execution_timeout,omitempty"`
+ RetryDelay OneOfnullTimeDelta `json:"retry_delay,omitempty"`
```
----------------------------------------------------------------
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 #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#issuecomment-674565467
CI is not happy: :-/ Can you take a look at it?
```
FAILED tests/api_connexion/endpoints/test_dag_endpoint.py::TestGetDagDetails::test_should_response_200
FAILED tests/api_connexion/endpoints/test_dag_endpoint.py::TestGetDagDetails::test_should_response_200_serialized
FAILED tests/api_connexion/endpoints/test_task_endpoint.py::TestGetTask::test_should_response_200
FAILED tests/api_connexion/endpoints/test_task_endpoint.py::TestGetTask::test_should_response_200_serialized
FAILED tests/api_connexion/endpoints/test_task_endpoint.py::TestGetTasks::test_should_response_200
```
----------------------------------------------------------------
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 #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#issuecomment-683781997
@houqp Could this cause a defect in API client for go client?
----------------------------------------------------------------
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 #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#discussion_r480959230
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1447,8 +1447,9 @@ components:
readOnly: true
nullable: true
state:
- $ref: '#/components/schemas/DagState'
readOnly: True
+ oneOf:
+ - $ref: '#/components/schemas/DagState'
Review comment:
am i correct that `readOnly: True` was ignored as sibling element of `$ref` without this change?
----------------------------------------------------------------
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] jpmarques66 commented on pull request #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
jpmarques66 commented on pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#issuecomment-681032711
I'm currently trying to solve the tests. Waiting for input from connexion team https://github.com/zalando/connexion/issues/1284 .
----------------------------------------------------------------
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] jpmarques66 commented on a change in pull request #10351: Migrate speccy to spectral in OpenAPI linting.
Posted by GitBox <gi...@apache.org>.
jpmarques66 commented on a change in pull request #10351:
URL: https://github.com/apache/airflow/pull/10351#discussion_r481175895
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1447,8 +1447,9 @@ components:
readOnly: true
nullable: true
state:
- $ref: '#/components/schemas/DagState'
readOnly: True
+ oneOf:
+ - $ref: '#/components/schemas/DagState'
Review comment:
@houqp So that was the idea in the beginning since spectral started complaining about that (having siblings of $ref). The [docs](https://swagger.io/docs/specification/using-ref/) refer that siblings are ignored, but it seems that the code generation isn't prepared for this.
I'm still a bit unfamiliar with the project, do you think this should be reverted?
----------------------------------------------------------------
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