Skip to content

Conversation

@pruthvitd
Copy link
Member

@pruthvitd pruthvitd commented Nov 3, 2025

The objective is to ensure reliable and idempotent creation or update of BackupStorageLocation (BSL) resources, while enabling safe retrieval using client Get and List operations. This helps verify that spec changes—such as updates to caCert during operator upgrade rollouts triggered by the Multi Cluster Operator (MCO)—are correctly applied.

@pruthvitd pruthvitd marked this pull request as ready for review December 8, 2025 05:54
Copy link
Member

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

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

@pruthvitd some nits, maybe better addressed before the merge.

Also the PR title and description seems misleading, for example it is expanding MCO as MachineConfigOperator whereas in this situation I think it is the multicluster operator that updates certificates in this space. I guess the change here is to ensure BSL is updated with latest parameters (specifically certificates) as per the provided configuration. Please update the PR description (and also add it to the commit message for git log readability)

annotations map[string]string,
) (*velero.BackupStorageLocation, *velero.Backup, error) {
return backupRequestCreate(
bsl, backup, err := backupRequestCreate(
Copy link
Member

Choose a reason for hiding this comment

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

(nit) Why this cleanup or change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for checking! This change was part of the code refactoring to handle both creation and updates of the BSL in a single flow. This avoids the need for an additional helper function and keeps the logic simpler.
Open to discussing alternatives if you feel a different approach would work better — happy to adjust.

@pruthvitd
Copy link
Member Author

@pruthvitd some nits, maybe better addressed before the merge.

Also the PR title and description seems misleading, for example it is expanding MCO as MachineConfigOperator whereas in this situation I think it is the multicluster operator that updates certificates in this space. I guess the change here is to ensure BSL is updated with latest parameters (specifically certificates) as per the provided configuration. Please update the PR description (and also add it to the commit message for git log readability)

Thanks for correcting, PR description is updated.

The objective is to ensure reliable and idempotent creation or update of
BackupStorageLocation (BSL) resources, while enabling safe retrieval using
client Get and List operations. This helps verify that spec changes—such
as updates to caCert during operator upgrade rollouts triggered by the
Multi Cluster Operator (MCO)—are correctly applied.

Signed-off-by: pruthvitd <prd@redhat.com>
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