Skip to content

Initial overlay example for Keda and Structure pre-commit hook#65897

Open
bugraoz93 wants to merge 8 commits intoapache:mainfrom
bugraoz93:kustomize-poc
Open

Initial overlay example for Keda and Structure pre-commit hook#65897
bugraoz93 wants to merge 8 commits intoapache:mainfrom
bugraoz93:kustomize-poc

Conversation

@bugraoz93
Copy link
Copy Markdown
Contributor

@bugraoz93 bugraoz93 commented Apr 26, 2026


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)
    Claude Opus 4.7

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@boring-cyborg boring-cyborg Bot added area:dev-tools area:helm-chart Airflow Helm Chart backport-to-v3-2-test Mark PR with this label to backport to v3-2-test branch labels Apr 26, 2026
@bugraoz93 bugraoz93 removed the backport-to-v3-2-test Mark PR with this label to backport to v3-2-test branch label Apr 26, 2026
Copy link
Copy Markdown
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Some comments, but in general looking good. Waiting for more feedback on the PR and then OK to merge. Comments are just minor, no force.

Would also request feedback from @wolfdn and @Miretpl of course :-D

Comment thread chart/kustomize-overlays/CONTRIBUTING.rst
Comment thread chart/kustomize-overlays/CONTRIBUTING.rst
Comment thread chart/kustomize-overlays/CONTRIBUTING.rst Outdated
Comment thread chart/kustomize-overlays/CONTRIBUTING.rst Outdated
Comment thread chart/kustomize-overlays/README.rst Outdated
Comment thread chart/kustomize-overlays/README.rst
@bugraoz93
Copy link
Copy Markdown
Contributor Author

Some comments, but in general looking good. Waiting for more feedback on the PR and then OK to merge. Comments are just minor, no force.

Thanks Jens for your comments!

Would also request feedback from @wolfdn and @Miretpl of course :-D

💯

@wolfdn
Copy link
Copy Markdown
Contributor

wolfdn commented Apr 29, 2026

Looks good to me! That's a nice approach to make the Helm chart more lean 😊 and it seems that tools like Helmfile can also deploy such Kustomizations alongside the Helm chart: https://helmfile.readthedocs.io/en/latest/advanced-features/#deploy-kustomizations-with-helmfile
I didn't try this feature out yet, but this would probably even allow deploying everything with a single command. Looking forward to try out this feature!

@bugraoz93
Copy link
Copy Markdown
Contributor Author

Looks good to me! That's a nice approach to make the Helm chart more lean 😊 and it seems that tools like Helmfile can also deploy such Kustomizations alongside the Helm chart: https://helmfile.readthedocs.io/en/latest/advanced-features/#deploy-kustomizations-with-helmfile
I didn't try this feature out yet, but this would probably even allow deploying everything with a single command. Looking forward to try out this feature!

Thanks Daniel for the review and feedback! Next step is to add smoke test after this PR so we will have tested overlay. We can start using once status.yaml is in the tested phase with the smoke test PR :)

@bugraoz93 bugraoz93 changed the title [WIP] Initial overlay example for Keda and Structure pre-commit hook Initial overlay example for Keda and Structure pre-commit hook Apr 29, 2026
Copy link
Copy Markdown
Contributor

@Miretpl Miretpl left a comment

Choose a reason for hiding this comment

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

Looks really nice, just small comments.

Looks good to me! That's a nice approach to make the Helm chart more lean 😊 and it seems that tools like Helmfile can also deploy such Kustomizations alongside the Helm chart: https://helmfile.readthedocs.io/en/latest/advanced-features/#deploy-kustomizations-with-helmfile
I didn't try this feature out yet, but this would probably even allow deploying everything with a single command. Looking forward to try out this feature!

I didn't check it too, but at the end, if it were possible to deploy Helm + Kustomize with a single command, it would be a very nice addition - worth investigating for sure.

Comment thread chart/kustomize-overlays/keda/README.rst Outdated
Comment thread chart/kustomize-overlays/keda/README.rst Outdated
Comment thread chart/kustomize-overlays/keda/README.rst Outdated
Comment thread chart/kustomize-overlays/keda/README.rst Outdated
Comment thread chart/kustomize-overlays/CONTRIBUTING.rst Outdated
Comment thread chart/kustomize-overlays/CONTRIBUTING.rst Outdated
triggerer, workers).
* Removing it requires changes to Airflow's own configuration.
* It has no external owner.
* It is assumed that the larger majority (>80%) will need and use this function for productive use
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* It is assumed that the larger majority (>80%) will need and use this function for productive use
* It is used by the larger majority of users (>80%) for production-ready environments

Maybe we should also determine it by the possible usage of the feature in terms of where it will be used (prod or non-prod env)? WDYT?

Copy link
Copy Markdown
Contributor Author

@bugraoz93 bugraoz93 May 5, 2026

Choose a reason for hiding this comment

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

Everything here can be used in production, right? I am not sure if we should make the environmental difference for them or think about it and give the user which environment to use on which level. How about like this?

Suggested change
* It is assumed that the larger majority (>80%) will need and use this function for productive use
* It is used by the larger majority of users (>80%) who will need and use this function for productive use

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. I had in mind the case of the current postgresql, which was meant only for development, but as it was available for users, they just used it. I fully agree that there should not be a distinction between environments, and if something is added, we assume that it can be used for dev or prod environments.

Copy link
Copy Markdown
Contributor

@jscheffl jscheffl May 5, 2026

Choose a reason for hiding this comment

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

Ah you were more nit-picky on the wording that I prepared the proposal :-D Yes of course can and should be used in dev/test as well as production! I wanted to express rather: 80% need it. (then if they need it probably in all stages)

So in short it can be:

Suggested change
* It is assumed that the larger majority (>80%) will need and use this function for productive use
* It is used by the larger majority of users (>80%)

Comment thread chart/kustomize-overlays/README.rst
Comment thread chart/kustomize-overlays/README.rst Outdated
Comment thread chart/kustomize-overlays/README.rst Outdated
@bugraoz93 bugraoz93 marked this pull request as ready for review May 5, 2026 17:36
@bugraoz93 bugraoz93 requested review from Miretpl and jscheffl May 5, 2026 17:37
Copy link
Copy Markdown
Contributor

@Miretpl Miretpl left a comment

Choose a reason for hiding this comment

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

Despite our one comment left - LGTM!

Copy link
Copy Markdown
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Cool!

Copy link
Copy Markdown
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nice (with comments)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants