shahar1 commented on code in PR #65758:
URL: https://github.com/apache/airflow/pull/65758#discussion_r4500790491
##########
chart/values.schema.json:
##########
@@ -12134,6 +12134,11 @@
"type": "integer",
"default": 1
},
+ "link": {
+ "description": "Name of the directory where
git-sync will create the symlink to the checked out repository. This allows
customization of the DAG mount path structure.",
+ "type": "string",
Review Comment:
**Minor.** The schema accepts any string, but only a relative path gives a
working Dag folder.
`airflow_dags` always joins this value under the mount path, while git-sync
resolves it against `/git` and also accepts absolute paths. With `link:
/tmp/dags`, git-sync publishes the symlink outside the shared volume and
`dags_folder` renders as `/opt/airflow/dags//tmp/dags/tests/dags`. The install
succeeds and no Dags are parsed.
From reading `publishSymlink` in git-sync v4.4.2 (not run): `link: .`
targets `/git` itself, and `link: .worktrees` collides with git-sync's own
checkout directory, so publishing should fail on every sync.
A `pattern` rejects these at install time, with a schema test to cover it:
```json
"pattern": "^[A-Za-z0-9_-][A-Za-z0-9._-]*$"
```
It also rejects `link: ""`, which now falls back to `repo` without saying
so. It rejects nested paths like `a/b` too, which git-sync would accept; loosen
it if you want to support those.
##########
chart/docs/manage-dag-files.rst:
##########
@@ -108,6 +108,10 @@ This option will always use running Git-Sync sidecar on
every dag-processor, wor
The Git-Sync sidecar containers will sync Dags from a git repository every
configured number of
seconds. If you are using the ``KubernetesExecutor``, Git-Sync will run as an
init container on your worker pods.
+If you want the synced files to appear under a different directory name than
``repo`` inside the Dag mount path,
+set ``dags.gitSync.link`` in your ``values.yaml``. For example, ``link:
myrepo`` will mount Dags under
+``$AIRFLOW_HOME/dags/myrepo`` instead of ``$AIRFLOW_HOME/dags/repo``.
Review Comment:
**Minor.** This paragraph does not say that `link` only renames the
directory. It cannot remove the level, and that is the case the discussion here
starts from: Dags sitting directly under the Dag folder. Someone trying that
with `link: ""` gets `repo` back with no error.
The example path also leaves out `subPath`, and the option applies equally
to the "with persistence" section above. Suggested wording:
```rst
To use a directory name other than ``repo``, set ``dags.gitSync.link``. For
example, ``link: myrepo``
places Dags under ``$AIRFLOW_HOME/dags/myrepo/<subPath>``. This only renames
the directory: Git-Sync
always publishes a symlink inside the Dag mount path, so the level cannot be
removed.
```
##########
chart/values.yaml:
##########
@@ -4556,6 +4556,11 @@ dags:
# Should be "" if dags are at repo root
subPath: "tests/dags"
+ # Name of the directory where git-sync will create the symlink to the
checked out repository.
+ # This allows customization of the DAG mount path structure.
Review Comment:
**Nit.** AGENTS.md: "Write **Dag** (title case) in all prose." The same
sentence is in the `values.schema.json` description.
```suggestion
# This allows customization of the Dag folder path.
```
##########
chart/templates/_helpers.yaml:
##########
@@ -246,9 +246,9 @@ If release name contains chart name it will be used as a
full name.
- name: GITSYNC_ROOT
value: "/git"
- name: GIT_SYNC_DEST
- value: "repo"
+ value: {{ default "repo" .Values.dags.gitSync.link | quote }}
Review Comment:
**Minor.** The new tests cover `airflow_dags` only, so nothing checks that a
custom `link` reaches the sidecar. If these two env vars regressed,
`dags_folder` would point at a link git-sync never creates, and every test
would still pass.
AGENTS.md: "Every changed or added behaviour must have a test; every test
must fail without the PR's change."
For example, in
`helm-tests/tests/helm_tests/other/test_git_sync_scheduler.py`:
```python
def test_custom_link_is_passed_to_git_sync(self):
docs = render_chart(
values={
"executor": "LocalExecutor",
"dags": {"gitSync": {"enabled": True, "link": "myrepo"}},
},
show_only=["templates/scheduler/scheduler-deployment.yaml"],
)
env = jmespath.search("spec.template.spec.containers[?name=='git-sync']
| [0].env", docs[0])
assert {"name": "GIT_SYNC_DEST", "value": "myrepo"} in env
assert {"name": "GITSYNC_LINK", "value": "myrepo"} in env
```
--
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]
