On Fri, Feb 28, 2020 at 5:01 PM Ash Berlin-Taylor <a...@apache.org> wrote:
>
> To view the API spec
> <https://github.com/PolideaInternal/airflow/blob/1e3e444d03c2e08d8fcee274d9e745a3d3ddeca8/openapi.yaml>
> in an easier to consume form I used https://editor.swagger.io/ and imported
> from the (raw) github URL
> Some comments on the proposed OpenAPI spec;
>
> connection id's are not unique, so get/delete might operate on multiple
> entries. (This has caused some confusion in the past, but is probably a
> separate issue to adding a REST API)
We have conn_id and connection_id. connection_id refers to the ID
field in the connection table. This field is unique.
> dag_id is listed as "integer" type. it should be string. Same applies to most
> other id types it appears.
Fixed. Thanks. CTRL+C/CTRL+V mistake
>
> PATCH dag: fileloc shouldn't be writable (it's a security risk), is_active
> and is_subdag are an internal field and shouldn't be writable either.
Fixed.
>
> PATCH dag: - Did you think about/rule out having separate /dag/{dag_id}/pause
> and unpause methods?
>
I do not see such a need. We can do the same through the PATCH method
and we won't have to create separate endpoints. Creating separate
endpoints makes sense if you need to perform additional actions to
modify the state and this is not a simple operation. For example, if
you have a virtual machine then it makes more sense to create a
separate endpoint to turn on the server and turn it off. For example,
if you have a virtual machine, then it makes more sense to create a
separate endpoint for turning the server on and off. This allowed for
returning the identifier of the long-term operation in response. The
PATCH /resource endpoint should always return copies of the modified
object, so this would not be possible.
> I don't think taskReschedule should be exposed directly at all - it's an
> internal detail for sensors in a specific state.
>
Removed.
> PATCH variables - update_mask parameter: The way I've seen other APIs do this
> is PATCH vs PUT requests. PATCH should only update the specified fields, PUT
> is "here is the entire representation, make it look like this"
The API in Google is designed in a similar way. This allows us to
avoid creating an additional endpoint. Users very often confuse PATCH
and PUT, so explicitly adding a update_mask field solves this problem.
https://cloud.google.com/data-catalog/docs/reference/rest/v1beta1/projects.locations.entryGroups.entries/patch
>
> PATCH variables - is_encrypted is exposed. It shouldn't be. It is not
> user-setable (it has is not writable in the UI anymore)
>
Removed
> GET xComValues: This endpoint support reading resources across multiple Task
> Instancce by specify a "-" as a !! - as a separator is not going to work.
> That is a valid character in task ids.
In my opinion, we can block the creation of tasks that have the exact
name "-". I don't think anyone consciously creates tasks with that
name. However, this may be the result of a developer error.
>
> xComValues -- I feel the "values" is out of place/not needed.
>
The REST API is based on the concept of resources Resource = things.
However, XCOM is not the name of the things, but the acitivites in
Airflow. XCOM comes from the word "X-communication". Communication is
a things not things. I would prefer endpoint names to be based on
things rather than activities. This will be easier to understand for
people who are using the API for the first time.
> -ash
> (The thought of maintaining that swagger/openapi spec file is honestly a bit
> daunting and making me lean more towards adding spec validation to FAB than
> using connexion - specifically because it's a: huge and b: lives separately
> to the code/endpoints)
> On Feb 28 2020, at 3:15 pm, Ash Berlin-Taylor <a...@apache.org> wrote:
> > Great work Kamil, I can't wait till we have a fully featured API in Airflow!
> >
> > So AIP-13 has been voted on and "accepted". Do you have a proposal for what
> > we should do with that AIP that we've already voted upon?
> > In the permissions section you talk about creating, for example
> > "can_edit_variable" -- but on which view? Permissions in FAB are a on a
> > specific view -- see
> > https://github.com/dpgaspar/Flask-AppBuilder/blob/120bc3ca38c4559a0099fe3b1a1badb319b6546e/flask_appbuilder/security/sqla/models.py#L71-L78
> > "There are not many FAB REST API experts" -- there aren't that many
> > connexion experts either - has anyone involved with the project used
> > connexion before?. It's a wildly inaccurate metric, but
> > https://hugovk.github.io/top-pypi-packages/ lists FAB in position 980 and
> > connexion in 1146, so they are both about as popular as each other.
> >
> > On Feb 26 2020, at 4:40 pm, Kamil Breguła <kamil.breg...@polidea.com> wrote:
> > > Hello,
> > >
> > > I just created "AIP-32 - Airflow REST API" proposal and would love
> > > community feedback and comments.
> > > https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-32%3A+Airflow+REST+API
> > > I would love to know what is your expectation from the API.
> > >
> > > We currently have one experimental API, but despite its existence for
> > > 2 years, it has not reached a stable level. There are many critical
> > > aspects to this implementation including no defined schema, very
> > > narrow range of functions, unsafe default configuration, no HATEOAS,
> > > and many others.
> > >
> > > In the past, Drewstone began work on REST API based on OpenAPI. It's
> > > described by AIP-13: OpenAPI 3 based API definition. However, it was
> > > not completed due to the lack of interest from the author and the
> > > Kerberos configuration problem (It was at a time when Breeze was still
> > > developing, so configuring all dependencies, including Kerberos, was a
> > > problem). It had a narrow range of features that are expected by users
> > > e.g. access to XCOM data and management for connection is missing,
> > >
> > > The Polidea and Google teams together with the community want to make
> > > another attempt based on our and community experience. Airflow
> > > deserves new stable solutions.
> > >
> > > Any comments and feedback/discussion in the AIP-32 document are welcome!
> > > Best regards,
> > > Kamil Breguła
> > >
> >
>
