pierrejeambrun commented on code in PR #63884: URL: https://github.com/apache/airflow/pull/63884#discussion_r3155158544
########## airflow-core/src/airflow/ui/src/components/Clear/useRerunWithLatestVersion.ts: ########## @@ -0,0 +1,76 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useEffect, useRef, useState } from "react"; + +import { useConfig } from "src/queries/useConfig"; + +type UseRerunWithLatestVersionProps = { + /** DAG-level rerun_with_latest_version from DAG details. */ + dagLevelConfig?: boolean | null; + /** + * Default to use when neither DAG-level nor global config is set. + * Use `false` for clear/rerun (the historical default) and `true` for backfills. + */ + fallback?: boolean; +}; + +type UseRerunWithLatestVersionResult = { + /** + * The resolved value. `undefined` means config has not loaded yet; + * the caller should omit run_on_latest_version from the request + * so the backend hierarchy applies. + */ + setValue: (newValue: boolean) => void; + value: boolean | undefined; +}; + +/** + * Resolves the default checkbox state for "Run on Latest Version". + * Precedence: DAG-level > global config > fallback. + * + * Returns `undefined` until the config is resolved, so the clear + * request can omit the field and let the backend hierarchy decide. + * Once resolved (or if the user toggles the checkbox), returns a boolean. + */ +export const useRerunWithLatestVersion = ({ + dagLevelConfig, + fallback = false, +}: UseRerunWithLatestVersionProps): UseRerunWithLatestVersionResult => { + const globalConfigValue = useConfig("rerun_with_latest_version") as boolean | undefined; + + const resolvedDefault = dagLevelConfig ?? globalConfigValue ?? fallback; + + const [value, setValue] = useState<boolean | undefined>(undefined); + const userHasToggled = useRef(false); + + useEffect(() => { + const hasResolvedConfig = dagLevelConfig !== undefined || globalConfigValue !== undefined; + + if (!userHasToggled.current && hasResolvedConfig) { Review Comment: Our [code-review guide](https://github.com/apache/airflow/blob/main/.github/instructions/code-review.instructions.md#ui-code-reacttypescript) flags `useState + useEffect` to sync derived state — could we do this with a nullable-override pattern instead? ```ts const [override, setOverride] = useState<boolean | undefined>(undefined); const value = override ?? dagLevelConfig ?? globalConfigValue ?? fallback; const setValue = (newValue: boolean) => setOverride(newValue); return { setValue, value }; ``` Drops the `useEffect`, the `useRef`, and the "value is undefined until config loads" intermediate state — and avoids the controlled-vs-uncontrolled `<Checkbox>` flicker on first render. ########## airflow-core/src/airflow/ui/src/components/DagActions/RunBackfillForm.tsx: ########## @@ -18,13 +16,17 @@ * specific language governing permissions and limitations * under the License. */ + +/* eslint-disable max-lines */ Review Comment: Nit: the previous version had a justification ("form aggregates date range, reprocess behavior, and param controls in a single UX flow"). Worth keeping that — bare `eslint-disable` with no rationale rots faster. ########## airflow-core/src/airflow/ui/src/components/DagActions/RunBackfillForm.tsx: ########## @@ -234,7 +247,7 @@ const RunBackfillForm = ({ dag, onClose }: RunBackfillFormProps) => { control={control} name="run_on_latest_version" render={({ field }) => ( - <Checkbox checked={field.value} colorPalette="brand" onChange={field.onChange}> + <Checkbox checked={field.value ?? undefined} colorPalette="brand" onChange={field.onChange}> Review Comment: Nit: `field.value` is already `boolean | undefined`, so `?? undefined` is a no-op. Just `checked={field.value}`. ########## airflow-core/docs/administration-and-deployment/dag-bundles.rst: ########## @@ -139,6 +139,107 @@ are configured so that impersonated users can access bundle files created by the the need for shared group permissions. +Configuring Default Rerun Version Behavior +------------------------------------------ + +When a user clears a DAG run or task instance, the UI shows a checkbox asking whether to rerun +with the latest bundle version or with the version the original run used. The +``rerun_with_latest_version`` setting controls the default state of that checkbox, so teams don't +have to make that decision manually every time. The same setting also governs the default +``run_on_latest_version`` behavior when creating backfills via the API or CLI. + +.. note:: + + This only applies to versioned bundle types (like ``GitDagBundle``). Local bundles + (``LocalDagBundle``) do not support versioning and will always use the latest code. + +How It Works +~~~~~~~~~~~~ + +Each DAG has a **parsed version** (``DagModel.bundle_version``), updated every time the dag +processor re-parses the DAG file. Each DAG run records the bundle version it was created with. + +When ``rerun_with_latest_version`` is **False** (the default), clearing a DAG run preserves its +original bundle version, so the rerun uses the same code. This provides reproducibility when +debugging failures. When **True**, clearing updates the DAG run to the current parsed version, +ensuring the most recent code is used on rerun. + +The setting is resolved using the following precedence (highest to lowest): + +1. **Explicit request**: The ``run_on_latest_version`` parameter in the API request body (if provided) +2. **DAG-level**: The DAG's ``rerun_with_latest_version`` parameter (if ``True`` or ``False``) +3. **Global config**: The ``[core] rerun_with_latest_version`` option (if set) +4. **Default**: ``False`` Review Comment: The newsfragment correctly says "Default: ``False`` for clear/rerun, ``True`` for backfills". This precedence table just says "Default: False" though — readers looking up the backfill behavior here will get the wrong answer. Could you mirror the newsfragment phrasing? ########## airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py: ########## @@ -2429,6 +2429,76 @@ def test_custom_timetable_generate_run_id_for_manual_trigger(self, dag_maker, te assert run.dag_id == custom_dag_id +class TestResolveRunOnLatestVersion: Review Comment: Two things: 1. Nit: each test method has `from airflow.api_fastapi.core_api.services.public.common import resolve_run_on_latest_version` inside the function body. Move to the top of the file like the other imports. 2. The resolver is well-covered as a unit, but there's no test that hitting `/dagRuns/{id}/clear` (or `/clearTaskInstances`, or `/backfills`) with no `run_on_latest_version` actually triggers the resolver end-to-end. Worth a small test on at least one endpoint to lock that wiring in. ########## airflow-core/src/airflow/ui/src/components/DagActions/RunBackfillForm.tsx: ########## @@ -56,19 +58,30 @@ const RunBackfillForm = ({ dag, onClose }: RunBackfillFormProps) => { const [formError, setFormError] = useState(false); const initialParamsDict = useDagParams(dag.dag_id, true); const { conf } = useParamStore(); - const { control, handleSubmit, reset, watch } = useForm<BackfillFormProps>({ + const { data: dagDetails } = useDagServiceGetDagDetails({ dagId: dag.dag_id }); + const { value: resolvedRunOnLatest } = useRerunWithLatestVersion({ + dagLevelConfig: dagDetails?.rerun_with_latest_version, + fallback: true, + }); + const { control, handleSubmit, reset, setValue, watch } = useForm<BackfillFormProps>({ defaultValues: { conf, dag_id: dag.dag_id, from_date: "", max_active_runs: 1, reprocess_behavior: "none", run_backwards: false, - run_on_latest_version: true, + run_on_latest_version: undefined, to_date: "", }, mode: "onBlur", }); + + useEffect(() => { Review Comment: Same anti-pattern here. `react-hook-form`'s `useForm({ values: ... })` is the idiomatic way to keep a form field in sync with async-loaded data — re-syncs without an explicit effect. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
