Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support symbolic linked pipelines to avoid repetition #580

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

daconstenla
Copy link
Contributor

why

While using kpops and trying to reduce the amount of copy/pasting required to configure multi-tenant configuration I found that symbolic links would cause kpops execution to break.

what

Replaces existing Path.blob with blob.blob and resolves the real file location so no other changes are required for it to be compatible with existing logic.

Note

In python 3.13 symbolic links are supported docs.python.org/3.13#pathlib.Path.glob, probably it would be as easy as follows:
yield from sorted(pipeline_path.glob(f"**/{PIPELINE_YAML}", recurse_symlinks=True))

@daconstenla daconstenla force-pushed the feat-support-symlinks-pipelines branch from 021a935 to e331a09 Compare January 10, 2025 15:29
@disrupted
Copy link
Member

Note

In python 3.13 symbolic links are supported docs.python.org/3.13#pathlib.Path.glob, probably it would be as easy as follows: yield from sorted(pipeline_path.glob(f"**/{PIPELINE_YAML}", recurse_symlinks=True))

that's a good head-up. how about a short todo comment in the code to refactor this once we drop support for older Python versions?

Copy link
Member

@disrupted disrupted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please consider adding a test for it. If you need any help let me know

@disrupted disrupted added type/enhancement New feature or request component/generator Related to the generation of pipeline.yaml labels Jan 13, 2025
@daconstenla
Copy link
Contributor Author

Note
In python 3.13 symbolic links are supported docs.python.org/3.13#pathlib.Path.glob, probably it would be as easy as follows: yield from sorted(pipeline_path.glob(f"**/{PIPELINE_YAML}", recurse_symlinks=True))

that's a good head-up. how about a short todo comment in the code to refactor this once we drop support for older Python versions?

Sure I can add a note, I actually had one originally but I moved it to the description of the pull-request.

@daconstenla
Copy link
Contributor Author

please consider adding a test for it. If you need any help let me know

How would you suggest we test it? creating a pipeline that's just a symlink of another and expect same result? 🤔

@daconstenla daconstenla force-pushed the feat-support-symlinks-pipelines branch from e331a09 to 7f809e8 Compare January 13, 2025 14:41
@disrupted
Copy link
Member

please consider adding a test for it. If you need any help let me know

How would you suggest we test it? creating a pipeline that's just a symlink of another and expect same result? 🤔

yes. feel free to commit the symlink to https://github.com/bakdata/kpops/tree/main/tests/pipeline/resources

@daconstenla daconstenla force-pushed the feat-support-symlinks-pipelines branch from 7f809e8 to fef7b47 Compare January 13, 2025 14:44
@disrupted
Copy link
Member

disrupted commented Jan 14, 2025

the new test itself would make sense in https://github.com/bakdata/kpops/blob/main/tests/pipeline/test_generate.py

edit: I believe ideally we would need to cover the following scenarios:

  • kpops generate pipelines/regular-folder/symlinked-pipeline.yaml
  • kpops generate pipelines/symlinked-folder
  • kpops generate pipelines where pipelines contains symlinked folders

wdyt?

@daconstenla
Copy link
Contributor Author

the new test itself would make sense in https://github.com/bakdata/kpops/blob/main/tests/pipeline/test_generate.py

edit: I believe ideally we would need to cover the following scenarios:

  • kpops generate pipelines/regular-folder/symlinked-pipeline.yaml
  • kpops generate pipelines/symlinked-folder
  • kpops generate pipelines where pipelines contains symlinked folders

wdyt?

Mmmm, ok, more than I thought, let me try to cover those scenarios.

@daconstenla daconstenla force-pushed the feat-support-symlinks-pipelines branch from 4120a43 to cd3b55d Compare January 14, 2025 21:34
@daconstenla
Copy link
Contributor Author

the new test itself would make sense in https://github.com/bakdata/kpops/blob/main/tests/pipeline/test_generate.py
edit: I believe ideally we would need to cover the following scenarios:

  • kpops generate pipelines/regular-folder/symlinked-pipeline.yaml
  • kpops generate pipelines/symlinked-folder
  • kpops generate pipelines where pipelines contains symlinked folders

wdyt?

Mmmm, ok, more than I thought, let me try to cover those scenarios.

As suggested I've included 3 types of symlinks to the tests:

  • symlinked pipeline
  • symlinked folder
  • folder with symlinked folder and symlinked pipeline as well as a regular pipeline

the test just validates the rendered results is as the original sources of the symlinks.

Copy link
Member

@disrupted disrupted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your contribution

@daconstenla daconstenla force-pushed the feat-support-symlinks-pipelines branch from cd3b55d to 6ec2026 Compare January 15, 2025 18:38
@daconstenla
Copy link
Contributor Author

@disrupted I assume you'll merge it when ready, thank you for the inputs.

@disrupted disrupted merged commit b516ecf into bakdata:main Jan 16, 2025
disrupted added a commit that referenced this pull request Jan 16, 2025
disrupted pushed a commit that referenced this pull request Jan 16, 2025
## why 

While using kpops and trying to reduce the amount of copy/pasting
required to configure multi-tenant configuration I found that _symbolic
links_ would cause kpops execution to break.

## what 

Replaces existing `Path.blob` with `blob.blob` and resolves the real
file location so no other changes are required for it to be compatible
with existing logic.

> [!NOTE]
> In python 3.13 symbolic links are supported
[docs.python.org/3.13#pathlib.Path.glob](https://docs.python.org/3.13/library/pathlib.html#pathlib.Path.glob),
probably it would be as easy as follows:
> `yield from sorted(pipeline_path.glob(f"**/{PIPELINE_YAML}",
recurse_symlinks=True))`
disrupted added a commit that referenced this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/generator Related to the generation of pipeline.yaml type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants