Skip to content

Conversation

@GiovanniPaoloGibilisco
Copy link

This pull request overlaps with #2 , the approach followed here is to use a PUT request with a very simple payload which directly reflects the state change.

From the implementation point of view these two endpoints avoid re-writing the logic to interact with the dagbag and use apis exposed by the cli package to reuse as much as possible native airflow logic.

@benjamingregory
Copy link
Member

Thanks @GiovanniPaoloGibilisco! Assigning to @cwurtz for review.

@GiovanniPaoloGibilisco GiovanniPaoloGibilisco changed the title Endpoints to: get a specific dag from the id and to change paused status Endpoints to: get a specific dag from the id, change paused status, create and delete connections Jul 11, 2018
@GiovanniPaoloGibilisco
Copy link
Author

This last commit also adds the ability to POST and DELETE a connection by its uri

DELETE http://localhost:8080/api/v1/connections/your-id
POST http://localhost:8080/api/v1/connections

{
  "conn_id" : "your-id",
  "conn_uri": "http://your.service:8080"
 }

@cwurtz
Copy link
Contributor

cwurtz commented Jul 12, 2018

@GiovanniPaoloGibilisco, sorry for the delays in getting this merged, this is a plugin I've created at Astronomer. Been working toward getting a new platform release implemented. I should be free'd up early next week to give your PR a more thorough look through and get it merged.

Thanks for contributing to the plugin!

@cwurtz
Copy link
Contributor

cwurtz commented Jul 23, 2018

@GiovanniPaoloGibilisco Just took a look at your PR, sorry again for the delay. The DAG endpoints look good, however there is a problem with the connection endpoints. The idea of sharing the logic with the CLI (Or any part of core Airflow) is ideal. However one of the big problems in using the Airflow CLI is it was primary written as a human interface, not a programmable one. That's the major reason I started making an Airflow API rather than using the teamclairvoyant/airflow-rest-api-plugin one.

In this case, should a problem occur in deletion or creation of a connection, the CLI is only going to print the problem, but the return value will be the same regardless of whether it worked or not. So your change will always return {"status": "deleted"} or {"status": "deleted"} regardless of the result.

I think the best route for those endpoints would be to copy the functionality from the CLI rather trying to call it. Another option is using a subprocess (or maybe output buffering) and parse the output for the success/failure conditions. Either way opens up the possibility that changes to Airflow will break this plugin. If the output changes so will the string result checking. Or with coping and pasting the logic will get out of sync. I side more towards the copy/paste as it'll be more succinct/clear and consistent with the rest of the plugin.

If you want to break out the DAG changes from the connection I'll merge that no problem. However in it's current state I can't merge the connection changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants