Skip to content

Conversation

@SamWheating
Copy link
Contributor

closes: #30075

This allows incoming API requests to create DAGs in non-queued states (running, success, failed), which is more consistent with the documentation.

As-per the original issue, I'm not sure whether:

  • The code is wrong (in which case this is the correct fix)
  • The documentation generated from the OpenAPI spec is wrong (which is due to the reference overriding the readOnly attribute here, as described here).

Let me know what you think, and if you think that this functionality is undesirable then I can update my PR to simply fix the API Docs.

Tested in Breeze by creating several non-queued DAGRuns via the API.

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Mar 15, 2023
@SamWheating SamWheating force-pushed the sw-allow-setting-state-when-creating-dagrun branch from 06d8607 to f020847 Compare March 16, 2023 04:46
Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't allow users to create dagruns with states other than queued. I'm not seeing a protection from users creating dagruns with other states

@SamWheating
Copy link
Contributor Author

We shouldn't allow users to create dagruns with states other than queued. I'm not seeing a protection from users creating dagruns with other states

This goes back to my original question about whether this is a bug in the spec or a bug in the implementation.

If there's only one acceptable value for a field then there's no point in exposing it as a read-write field, and we should just keep this as read-only and fix the spec. I will open a separate PR for that, and you can merge one or the other.

@ephraimbuddy
Copy link
Contributor

We shouldn't allow users to create dagruns with states other than queued. I'm not seeing a protection from users creating dagruns with other states

This goes back to my original question about whether this is a bug in the spec or a bug in the implementation.

If there's only one acceptable value for a field then there's no point in exposing it as a read-write field, and we should just keep this as read-only and fix the spec. I will open a separate PR for that, and you can merge one or the other.

It's Ok to trigger a dagrun with a state of 'queued', run_type=manual just like we do in the UI. I think we should work on it instead of what you have in the other PR. WDYT?

@potiuk
Copy link
Member

potiuk commented Mar 18, 2023

Agree with @ephraimbuddy - we should not trigger "any" state and #30149 seems to handle it better.

@SamWheating
Copy link
Contributor Author

It's Ok to trigger a dagrun with a state of 'queued', run_type=manual just like we do in the UI. I think we should work on it instead of what you have in the other PR. WDYT?

I don't really see the point of having a field with only one acceptable value in the POST body, in this case I think it makes more sense to use a default value and mark it as read-only. Thoughts?

@potiuk
Copy link
Member

potiuk commented Mar 21, 2023

I don't really see the point of having a field with only one acceptable value in the POST body, in this case I think it makes more sense to use a default value and mark it as read-only. Thoughts?

Yes. With the exception of backwards compatibility. If removal of the state would cause previous requests to fail (Even wit t state "queued", then we should not remove it just make it optional and raise deprecation warning in case someone passes it..

@SamWheating
Copy link
Contributor Author

Yes. With the exception of backwards compatibility. If removal of the state would cause previous requests to fail (Even wit t state "queued", then we should not remove it just make it optional and raise deprecation warning in case someone passes it..

Agreed, but in this case requests which set the state field would previously fail with a 400, see example curl in the initial issue (#30075), so I think all we need to do here is fix up the spec + documents as per #30149

@potiuk
Copy link
Member

potiuk commented Mar 21, 2023

Yes. With the exception of backwards compatibility. If removal of the state would cause previous requests to fail (Even wit t state "queued", then we should not remove it just make it optional and raise deprecation warning in case someone passes it..

Agreed, but in this case requests which set the state field would previously fail with a 400, see example curl in the initial issue (#30075), so I think all we need to do here is fix up the spec + documents as per #30149

Ah. I see. yeah. Then it makes sense to remove it - it's not "API change" it's a bugfix.

@potiuk
Copy link
Member

potiuk commented Mar 21, 2023

Classic:

image

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

Labels

area:API Airflow's REST/HTTP API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set DagRun state in create Dagrun endpoint ("Property is read-only - 'state'")

4 participants