Uh oh!
There was an error while loading. Please reload this page.
[WIP] Deprecate dagrun conf and add params argument for the different APIs - #29174
[WIP] Deprecate dagrun conf and add params argument for the different APIs#29174hussein-awala wants to merge 16 commits into
conf and add params argument for the different APIs#29174Conversation
119ec1b to
c6c8faaCompareThis pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
jscheffl
left a comment
There was a problem hiding this comment.
I like in general the idea to replace the dag_run.confwith params. Som ecomments being added to PR.
What I don't really understand is, why are you adding params as a parallel path and dict in the DB and across all API Calls. Are you planning to keep both in parallel and redundant until dag_run.conf is dropped?
If yes, what happens if user apecifies both in parallel? Will dag_run.conf still pass values over parameters?
If yes, then I believe for the time until fully migrated/deprecated this will generate a lot of confusion for users.
In general I like the approach but the steps for migrating over are not 100% clear to me, might be good to agree with the comminuty which steps to take that majority agree.
| 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. | ||
| *New in version 2.6.0* |
There was a problem hiding this comment.
Needs a bit of update if you re-vitalize
| if conf: | ||
| warnings.warn( | ||
| "dag_run conf is deprecated. Please use params instead", DeprecationWarning, stacklevel=2 |
There was a problem hiding this comment.
Di we need to add a deprecation warning on CLI as well?
| document.getElementById('generated_json_toggle').addEventListener('click', () => { | ||
| setTimeout(jsonForm.refresh, 300); | ||
| }); | ||
| // // Ensure layout is refreshed on generated JSON as well |
There was a problem hiding this comment.
Why did you disable this? Was there an error in testing?
This is needed because the section is hidden during load of the form and then the JSON view is not correctly initialized - don't know why - workaround was to initialize it lazy by this.
| </div> | ||
| {% if recent_confs|length > 0 %} | ||
| {% if form_fields and recent_params|length > 0 %} | ||
| <div class="form-group row"> |
There was a problem hiding this comment.
Mhm, you really want to add two JSON elements here, to be able to trigger using a conf and a params dict? I'd propose to switch to params from conf in this PR, having two fields rather confuses which to use (in my eyes)
| </tbody> | ||
| </table> | ||
| {% for form_section, form_items in form_fields.values() | groupby(attribute="schema.section", default="") %} |
There was a problem hiding this comment.
Here you remove further sections being displayed... why?
closes: #29018
Deprecate dagrun
confargument and addparamsargument for the different Airflow APIs (UI, CLI, Python API, internal API and REST API), and add a check for the extra/missing params.