potiuk commented on code in PR #71527:
URL: https://github.com/apache/airflow/pull/71527#discussion_r3775001936
##########
dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:
##########
@@ -2509,12 +2510,39 @@ def deploy_cluster(
"aws://test:test@/?region_name=us-east-1&"
"endpoint_url=http%3A%2F%2Flocalstack.airflow.svc.cluster.local%3A4566"
)
-# The Go/Java SDKs are always built from upstream main so branches with stale
or missing
-# go-sdk/java-sdk copies still test current SDK sources. See
kubernetes-tests/lang_sdk/README.md.
+# Runs targeting a branch other than main build the Go/Java SDKs from upstream
main, so
+# release/backport branches with stale or missing go-sdk/java-sdk copies still
test current SDK
+# sources. See kubernetes-tests/lang_sdk/README.md.
LANG_SDK_UPSTREAM_GIT_URL = "https://github.com/apache/airflow.git";
LANG_SDK_UPSTREAM_REF = "main"
+def _lang_sdk_target_branch() -> str:
+ """Branch this run targets. GITHUB_BASE_REF for PRs, DEFAULT_BRANCH in CI,
else this checkout's."""
Review Comment:
This documents `DEFAULT_BRANCH` as the CI-side source, but
`_setup_lang_sdk_test` runs on the host and I can't find anything that sets
`DEFAULT_BRANCH` there: the `tests-kubernetes-lang-sdk` job in `k8s-tests.yml`
doesn't set it, and `shell_params.py:667` injects it only into the container
env dict — and from `self.airflow_branch`, so even when it is set it equals the
`AIRFLOW_BRANCH` fallback anyway.
Harmless either way, but either wire it into the job env or drop the claim
from the docstring and the README. Happy to be corrected if there's a path I
missed.
##########
dev/breeze/tests/test_kubernetes_lang_sdk_commands.py:
##########
@@ -374,3 +376,56 @@ def fake_parallel(steps, output):
"go_sdk": fake_go_sdk,
"java_sdk": fake_java_sdk,
}
+
+
+class TestLangSdkTargetBranch:
+ @pytest.mark.parametrize(
+ ("env", "expected"),
+ [
+ pytest.param({"GITHUB_BASE_REF": "main"}, "main",
id="pr-targeting-main"),
+ pytest.param({"GITHUB_BASE_REF": "v3-3-test"}, "v3-3-test",
id="pr-targeting-release"),
+ pytest.param({"DEFAULT_BRANCH": "v3-3-test"}, "v3-3-test",
id="ci-default-branch"),
+ pytest.param(
+ {"GITHUB_BASE_REF": "main", "DEFAULT_BRANCH": "v3-3-test"},
+ "main",
+ id="pr-target-wins-over-default-branch",
+ ),
+ pytest.param({}, "main", id="falls-back-to-this-checkouts-branch"),
Review Comment:
With both env vars unset `_lang_sdk_target_branch()` returns
`AIRFLOW_BRANCH`, which is `"v3-3-test"` on the branch this PR is labelled to
backport to — so this case asserts `"main"` against `"v3-3-test"` and fails
there. The test file's pre-image blob on `v3-3-test` is identical to main's, so
the hunk cherry-picks cleanly and lands the assertion verbatim.
`branch_defaults.py` exists for exactly this ("This file is different in
every branch"), and `test_shell_params.py:25` already imports it rather than
hardcoding:
```suggestion
pytest.param({}, AIRFLOW_BRANCH,
id="falls-back-to-this-checkouts-branch"),
```
(plus `from airflow_breeze.branch_defaults import AIRFLOW_BRANCH` at the top
of the file)
--
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]
