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

add schedule #317

Closed
wants to merge 2 commits into from
Closed

add schedule #317

wants to merge 2 commits into from

Conversation

AsfaMumtaz
Copy link
Contributor

No description provided.

Copy link

@AsfaMumtaz validation successful`

Copy link
Collaborator

@d3adb5 d3adb5 left a comment

Choose a reason for hiding this comment

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

I think we're missing updates to the README.md with this change. It still mentions backup.* in the values tables.

@@ -1307,7 +1307,7 @@ cronJob:
cpu: 1

# Take backup of application namespace
backup:
schedule:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of schedule we can be a little more explicit about what this is for in the value's own name.

Suggested change
schedule:
backupSchedule:

What do you think, @AsfaMumtaz?

##########################################################
backup:
schedule:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same suggestion as in the other thread I opened.

@d3adb5
Copy link
Collaborator

d3adb5 commented Jul 6, 2024

I forgot to mention one thing: this changes the interface in a way that is not backwards compatible, so it would require a major version bump. How about instead we detect whether .Values.backup is filled and use it if present as long as .Values.schedule (or .Values.backupSchedule) is empty? That would allow us to deprecate it, issue a warning in the NOTES.txt template, and in the next major version upgrade do away with the old option completely.

@AsfaMumtaz AsfaMumtaz closed this Jul 11, 2024
@rasheedamir rasheedamir deleted the change-bkup-to-schedule branch September 1, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants