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: add dags.gitSync.submodules value #620

Conversation

karakanb
Copy link
Contributor

@karakanb karakanb commented Jun 24, 2022

Signed-off-by: Burak Karakan burak.karakan@gmail.com

What issues does your PR fix?

  • N/A

What does your PR do?

This PR introduces a new value dags.gitSync.submodules (default: recursive) to control the submodule strategy for git-sync pods.

In my use case, the repo that is being cloned has a lot of submodules, but I would rather not init the submodules because it takes 20+ seconds, therefore I want to be able to disable the submodules to significantly speed up my tasks.

Checklist

For all Pull Requests

For releasing ONLY

…oyments

Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
@karakanb
Copy link
Contributor Author

@thesuperzapper should I update the version with this as well? would you consider this as a minor version or a patch version?

@stale
Copy link

stale bot commented Aug 31, 2022

This issue has been automatically marked as stale because it has not had activity in 60 days.
It will be closed in 7 days if no further activity occurs.

Thank you for your contributions.


Issues never become stale if any of the following is true:

  1. they are added to a Project
  2. they are added to a Milestone
  3. they have the lifecycle/frozen label

@stale stale bot added the lifecycle/stale lifecycle - this is stale label Aug 31, 2022
@karakanb
Copy link
Contributor Author

@thesuperzapper do you have any plans to merge / release this?

@stale stale bot removed the lifecycle/stale lifecycle - this is stale label Aug 31, 2022
@thesuperzapper thesuperzapper added this to Unsorted in Issue Triage and PR Tracking via automation Sep 1, 2022
@thesuperzapper thesuperzapper added this to the airflow-8.7.0 milestone Sep 1, 2022
@thesuperzapper
Copy link
Member

@karakanb thanks for the PR, don't worry, I haven't forgotten it!

As it adds value, it will need to be part of 8.7.0, my main goal for 8.7.0 is to have the task-aware autoscaler (#339), which is taking an incredible amount of work to get done but is progressing!

If enough feature PRs build up before then, we might cut 8.7.0 early which would probably include this PR.

@thesuperzapper thesuperzapper moved this from Unsorted to PR | Needs Review in Issue Triage and PR Tracking Sep 1, 2022
@karakanb
Copy link
Contributor Author

@thesuperzapper hey Mathew, I hope you are doing well. Do you happen to have a release schedule in mind? it feels like smaller changes like these can go as part of patch versions as long as they are backwards compatible, and would avoid blocking other smaller features until the big ones you'd like to have are ready, as well as avoiding the need to fork by others.

Would you be interested in releasing some patch versions with fixes like this, as well as support for newer k8s versions for issues such as #679?

@thesuperzapper
Copy link
Member

@thesuperzapper hey Mathew, I hope you are doing well. Do you happen to have a release schedule in mind? it feels like smaller changes like these can go as part of patch versions as long as they are backwards compatible, and would avoid blocking other smaller features until the big ones you'd like to have are ready, as well as avoiding the need to fork by others.

Would you be interested in releasing some patch versions with fixes like this, as well as support for newer k8s versions for issues such as #679?

@karakanb sorry about the delay in getting this PR in a release, I have been busy moving countries!

So I can prioritize what to include, what are the critical fixes/features which you would expect in the next release?

@karakanb
Copy link
Contributor Author

hey, no worries at all. Congrats on the country change, I hope all goes well for you! 🤞

for the next release, the only critical issue I have at the moment is this one: #679

afterwards, if a smaller release brought these that'd be nice

I believe the smaller releases with easy-to-upgrade versions would allow quick iteration + safe rollouts as every change would be gradually adopted by the community instead of bigger / riskier releases in that sense.

Thanks a lot for your efforts!

Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

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

@karakanb thanks for the PR, there are only a few minor changes (see comments) that I think we need.

Also, we should update the "Load Airflow DAGs" FAQ page to show the new submodules value in its examples.

charts/airflow/values.yaml Outdated Show resolved Hide resolved
charts/airflow/values.yaml Outdated Show resolved Hide resolved
@thesuperzapper thesuperzapper moved this from PR | Needs Review to PR | Waiting for Changes in Issue Triage and PR Tracking Feb 7, 2023
@thesuperzapper thesuperzapper changed the title feat: add a new option to control submodule strategy for gitSync depl… add dags.gitSync.submodules value Feb 7, 2023
Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
Signed-off-by: Burak Karakan <burak.karakan@gmail.com>
@karakanb
Copy link
Contributor Author

@thesuperzapper this one is ready as well, please give it a look.

Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

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

@karakanb thanks for the PR, I will be releasing this today in version 8.7.0 of the chart.

Issue Triage and PR Tracking automation moved this from PR | Waiting for Changes to PR | Ready to Merge Apr 6, 2023
@thesuperzapper thesuperzapper added the status/ready-to-merge status - this will be merged into next release label Apr 6, 2023
@thesuperzapper thesuperzapper changed the title add dags.gitSync.submodules value feat: add dags.gitSync.submodules value Apr 6, 2023
@thesuperzapper thesuperzapper merged commit 14a65e3 into airflow-helm:main Apr 7, 2023
Issue Triage and PR Tracking automation moved this from PR | Ready to Merge to Done Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/ready-to-merge status - this will be merged into next release
Development

Successfully merging this pull request may close these issues.

None yet

2 participants