Copilot commented on code in PR #63884: URL: https://github.com/apache/airflow/pull/63884#discussion_r3066480001
########## airflow-core/src/airflow/ui/src/components/Clear/useRerunWithLatestVersion.ts: ########## @@ -0,0 +1,68 @@ +/*! + * 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; +}; + +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 > false. + * + * 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, +}: UseRerunWithLatestVersionProps): UseRerunWithLatestVersionResult => { + const globalConfigValue = useConfig("rerun_with_latest_version") as boolean | undefined; + + const resolvedDefault = dagLevelConfig ?? globalConfigValue ?? false; + + const [value, setValue] = useState<boolean | undefined>(undefined); + const userHasToggled = useRef(false); + + useEffect(() => { + if (!userHasToggled.current && dagLevelConfig !== undefined) { Review Comment: The effect only initializes `value` when `dagLevelConfig !== undefined`. If DAG details are still loading (so `dagLevelConfig` stays `undefined`) but the global config loads, the hook will keep returning `undefined` and never apply the global default. Consider initializing when either (a) DAG-level config is known (including `null`) OR (b) the global config value is available, while still honoring `userHasToggled`. ```suggestion const hasResolvedConfig = dagLevelConfig !== undefined || globalConfigValue !== undefined; if (!userHasToggled.current && hasResolvedConfig) { ``` ########## airflow-core/src/airflow/api_fastapi/core_api/services/public/dag_run.py: ########## @@ -86,3 +89,17 @@ async def wait(self) -> AsyncGenerator[str, None]: await asyncio.sleep(self.interval) yield await self._serialize_response(dag_run := await self._get_dag_run()) yield "\n" + + +def resolve_run_on_latest_version( + explicit_value: bool | None, + dag_id: str, + session: Session, +) -> bool: + """Resolve run_on_latest_version: explicit > DAG-level > global config > False.""" + if explicit_value is not None: + return explicit_value + serialized = SerializedDagModel.get_dag(dag_id, session=session) + if serialized and serialized.rerun_with_latest_version is not None: + return serialized.rerun_with_latest_version + return conf.getboolean("core", "rerun_with_latest_version", fallback=False) Review Comment: `resolve_run_on_latest_version()` always performs a DB lookup for `SerializedDagModel` when `explicit_value` is omitted. In the clear endpoints you already load the DAG (or related metadata) earlier in the request; consider allowing the resolver to accept an optional pre-fetched `SerializedDagModel`/DAG-level value so callers can avoid an extra query on every clear operation. ########## airflow-core/src/airflow/ui/src/components/Clear/useRerunWithLatestVersion.test.tsx: ########## @@ -0,0 +1,119 @@ +/*! + * 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 { act, renderHook } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; + +import { useConfig } from "src/queries/useConfig"; + +import { useRerunWithLatestVersion } from "./useRerunWithLatestVersion"; + +// Mock useConfig to control the global config value +vi.mock("src/queries/useConfig", () => ({ + useConfig: vi.fn(), +})); + +const mockUseConfig = vi.mocked(useConfig); + +describe("useRerunWithLatestVersion — precedence", () => { + it("returns undefined before config is resolved", () => { + mockUseConfig.mockReturnValue(undefined); + + const { result } = renderHook(() => useRerunWithLatestVersion({ dagLevelConfig: undefined })); + + expect(result.current.value).toBeUndefined(); + }); Review Comment: There’s no test covering the async-loading sequence where `dagLevelConfig` remains `undefined` while the global config transitions from `undefined` to `true/false`. Adding a rerender-based test for that scenario would prevent regressions and would have caught the initialization issue in the hook logic. ########## airflow-core/newsfragments/63884.significant.rst: ########## @@ -0,0 +1,30 @@ +Add ``rerun_with_latest_version`` configuration for DAG bundle versioning + +When clearing or rerunning tasks, this setting controls whether the new DAG run +uses the latest bundle version or the original version from the initial run. +The setting follows a three-level precedence: + +1. **DAG-level**: ``rerun_with_latest_version`` parameter on the DAG definition. +2. **Global config**: ``[core] rerun_with_latest_version`` in ``airflow.cfg``. +3. **Default**: ``False`` (use the original bundle version). Review Comment: This changelog fragment describes a three-level precedence that omits the highest-priority explicit API request parameter (`run_on_latest_version`). The rest of the PR (description + docs + backend resolver) implements/advertises explicit > DAG > global > default; the newsfragment should be updated to match. -- 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]
