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: Configurable default package install sync period #1259

Conversation

imusmanmalik
Copy link
Contributor

@imusmanmalik imusmanmalik commented Jun 20, 2023

Which issue(s) this PR fixes:

It could resolve partially the issue.

Does this PR introduce a user-facing change?

Not directly but user can now have a configurable global default syncPeriod value for App created using PackageInstall.

Previously the value was hardcoded to 10m, it could be modified by specifying syncPeriod as part of PackageInstall manifest, However having a default global value could be useful for some use-cases where specifying a syncPeriod is tedious to every PackageInstall manifest.

The default 10m is still respected after having a discussion with the community / maintainers on Slack: https://kubernetes.slack.com/archives/CH8KCCKA5/p1686321873099489

* Controller configuration to specify `PackageInstallDefaultSyncPeriod`

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:

Docs PR: carvel-dev/carvel#658

@imusmanmalik imusmanmalik force-pushed the feat/defaultPackageInstallSyncPeriod branch from 4dfde9a to 6a5feb3 Compare June 20, 2023 18:59
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
@imusmanmalik imusmanmalik force-pushed the feat/defaultPackageInstallSyncPeriod branch from 6a5feb3 to 8776976 Compare June 20, 2023 19:00
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
@imusmanmalik imusmanmalik force-pushed the feat/defaultPackageInstallSyncPeriod branch from bd4596f to 729f8bc Compare June 21, 2023 13:03
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
@imusmanmalik imusmanmalik force-pushed the feat/defaultPackageInstallSyncPeriod branch from 7c939f9 to 62b5196 Compare June 21, 2023 15:06
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
@imusmanmalik imusmanmalik changed the title Draft: feat: Configurable default package install sync period feat: Configurable default package install sync period Jul 3, 2023
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

Just a few minor nits, looks good to me otherwise. Thank you so much for working on it @imusmanmalik 🚀

pkg/packageinstall/app_test.go Outdated Show resolved Hide resolved
pkg/packageinstall/app_test.go Outdated Show resolved Hide resolved
test/e2e/kappcontroller/config_test.go Show resolved Hide resolved
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
This reverts commit 4fa8ccc.

Signed-off-by: imusmanmalik <usmanmalik@engineer.com>
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

LGTM

@joaopapereira joaopapereira merged commit 6eb91df into carvel-dev:develop Jul 11, 2023
9 checks passed
@imusmanmalik imusmanmalik deleted the feat/defaultPackageInstallSyncPeriod branch July 11, 2023 23:08
@jessehu
Copy link
Contributor

jessehu commented Aug 25, 2023

Cool, Thx 👍

@github-actions github-actions bot added the carvel-triage This issue has not yet been reviewed for validity label Aug 25, 2023
@praveenrewar praveenrewar removed the carvel-triage This issue has not yet been reviewed for validity label Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants