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.

Sign up for GitHub

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

Jump to bottom

[velero] Bump velero version v1.10.0 #414

Merged
merged 6 commits into from
Dec 22, 2022
Merged

[velero] Bump velero version v1.10.0 #414

merged 6 commits into from
Dec 22, 2022

Conversation

qiuming-best
Copy link
Collaborator

@qiuming-best qiuming-best commented Oct 27, 2022 •
edited by jenting
Loading

Signed-off-by: Ming mqiu@vmware.com

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the values.yaml or README.md
  • Title of the PR starts with chart name (e.g. [velero])

@qiuming-best
Copy link
Collaborator Author

I'll keep it in draft until v1.10.0 is been released

@qiuming-best qiuming-best marked this pull request as draft October 27, 2022 03:58
@jenting jenting self-requested a review November 1, 2022 12:36
jenting
jenting requested changes Nov 1, 2022
View reviewed changes
Copy link
Collaborator

@jenting jenting left a comment

Choose a reason for hiding this comment

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

I believe the user would encounter many upgrade issues if there were many existing Velero CRs within the cluster.

charts/velero/Chart.yaml Outdated Show resolved Hide resolved
charts/velero/values.yaml Show resolved Hide resolved
charts/velero/values.yaml Show resolved Hide resolved
charts/velero/values.yaml Show resolved Hide resolved
charts/velero/values.yaml Outdated Show resolved Hide resolved
charts/velero/values.yaml Show resolved Hide resolved
charts/velero/crds/restores.yaml Show resolved Hide resolved
charts/velero/crds/backuprepositories.yaml Show resolved Hide resolved
@qiuming-best qiuming-best mentioned this pull request Nov 15, 2022
Closed
qiuming-best
qiuming-best commented Nov 18, 2022
View reviewed changes
charts/velero/crds/backuprepositories.yaml Show resolved Hide resolved
charts/velero/values.yaml Show resolved Hide resolved
charts/velero/templates/NOTES.txt Show resolved Hide resolved
charts/velero/templates/NOTES.txt Show resolved Hide resolved
@jenting
Copy link
Collaborator

jenting commented Nov 25, 2022

Since this PR introduced a bit of breaking backward compatibility because of the naming change.
We could consider fixing this issue #421 along with releasing velero v1.10.0 chart.

@qiuming-best @ywk253100 @reasonerjt @sseago @shubham-pampattiwar WDYT?

@CharlieC3
Copy link

If pre-existing values config names are changing and the new CRDs are not backwards compatible, I think the chart's MAJOR version number should be rev'd to stay in alignment with semantic versioning expectations (e.g. v2.0.0). Either one of these reasons would be enough to justify this.
It doesn't need to match the same version as the application it deploys, and most Helm charts don't for this very reason.

This, along with ample documentation in the README, should be enough to inform users that the two deployments are incompatible.
I'd love to be able to use a chart supporting the 1.10.0 Velero release, and hope that separating the CRDs to a different chart would be something that can be addressed in another PR to get this one merged sooner :)

@qiuming-best
Bump velero version v1.10.0
7484d8e 
Signed-off-by: Ming <mqiu@vmware.com>
@qiuming-best
Copy link
Collaborator Author

qiuming-best commented Dec 7, 2022 •
edited
Loading

Since this PR introduced a bit of breaking backward compatibility because of the naming change. We could consider fixing this issue #421 along with releasing velero v1.10.0 chart.

@qiuming-best @ywk253100 @reasonerjt @sseago @shubham-pampattiwar WDYT?

Based on our discussion with @jenting, currently, it's no need to separate CRDs, it could be solved by changing upgrade CRDs jobs into pre-install, pre-upgrade, and pre-rollback.

another thing that needs to be fixed in a later version is BSL, VSL, and Schedule CR would be recreated by hooks

@qiuming-best qiuming-best marked this pull request as ready for review December 7, 2022 08:07
@qiuming-best
bump up chart version to 3.0.0
dd98afd 
Signed-off-by: Ming <mqiu@vmware.com>
@jcpunk
Copy link

jcpunk commented Dec 16, 2022

Once this is merged, can it be submitted to artifiacthub.io?

@jenting
Copy link
Collaborator

jenting commented Dec 17, 2022

Once this is merged, can it be submitted to artifiacthub.io?

Yes, but why?

@jcpunk
Copy link

jcpunk commented Dec 19, 2022

I tend to use artifacthub as my starting point for finding helm charts. Having this posted there should help increase the visibility of this project.

@jenting
Copy link
Collaborator

jenting commented Dec 20, 2022

I tend to use artifacthub as my starting point for finding helm charts. Having this posted there should help increase the visibility of this project.

Create you create an issue for this request? Thank you.

@sdelrio
Copy link

sdelrio commented Dec 20, 2022

I tend to use artifacthub as my starting point for finding helm charts. Having this posted there should help increase the visibility of this project.

Create you create an issue for this request? Thank you.

It looks like it is already there:

https://artifacthub.io/packages/helm/vmware-tanzu/velero

jenting
jenting reviewed Dec 21, 2022
View reviewed changes
Copy link
Collaborator

@jenting jenting left a comment

Choose a reason for hiding this comment

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

@qiuming-best
All good, jut some suggestions.

charts/velero/values.yaml Outdated Show resolved Hide resolved
charts/velero/values.yaml Outdated Show resolved Hide resolved
charts/velero/values.yaml Outdated Show resolved Hide resolved
@qiuming-best
set default uploaderType to restic
e8e7958 
Signed-off-by: Ming <mqiu@vmware.com>
jenting
jenting approved these changes Dec 22, 2022
View reviewed changes
Copy link
Collaborator

@jenting jenting left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @qiuming-best

@jenting
Copy link
Collaborator

jenting commented Dec 22, 2022

@reasonerjt @ywk253100 PTAL

reasonerjt
reasonerjt approved these changes Dec 22, 2022
View reviewed changes
Copy link
Contributor

@reasonerjt reasonerjt left a comment

Choose a reason for hiding this comment

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

Have some trivial comments, but I think this PR looks great!

charts/velero/values.yaml
@@ -284,8 +284,12 @@ configuration:
# here if using a non-default value. The `velero server` default values are shown in the
# comments below.
# --------------------
# `velero server` default: restic
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to fix this in this PR, but if these values are to be set by the user, we better add more explanations in the comment

@qiuming-best qiuming-best merged commit 08bf56b into vmware-tanzu:main Dec 22, 2022
jenting added a commit that referenced this pull request Aug 13, 2023
@jenting
Remove helm hooks for the custom resources
42be236 
The PR #414 have moved the upgrade CRD job from post hook to pre hook to
ensure that the custom resources create/upgrade after the CRD
installed/upgraded.

Therefore, the helm hooks for the custom resources are no longer
required includes
- backupstoragelocations.velero.io
- volumesnapshotlocations.velero.io
- schedules.velero.io
@jenting jenting mentioned this pull request Aug 13, 2023
Merged
4 tasks
jenting added a commit that referenced this pull request Aug 13, 2023
@jenting
Remove helm hooks for the custom resources
c083572 
The PR #414 have moved the upgrade CRD job from post hook to pre hook to
ensure that the custom resources create/upgrade after the CRD
installed/upgraded.

Therefore, the helm hooks for the custom resources are no longer
required includes
- backupstoragelocations.velero.io
- volumesnapshotlocations.velero.io
- schedules.velero.io

Signed-off-by: JenTing Hsiao <hsiaoairplane@gmail.com>
jenting added a commit that referenced this pull request Aug 13, 2023
@jenting
Remove helm hooks for the custom resources
73f33fe 
The PR #414 have moved the upgrade CRD job from post hook to pre hook to
ensure that the custom resources create/upgrade after the CRD
installed/upgraded.

Therefore, the helm hooks for the custom resources are no longer
required includes
- backupstoragelocations.velero.io
- volumesnapshotlocations.velero.io
- schedules.velero.io

Signed-off-by: JenTing Hsiao <hsiaoairplane@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenting jenting jenting approved these changes

@reasonerjt reasonerjt reasonerjt approved these changes

@ywk253100 ywk253100 Awaiting requested review from ywk253100

Assignees

@qiuming-best qiuming-best

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@qiuming-best @jenting @CharlieC3 @jcpunk @sdelrio @reasonerjt