kubernetes-sigs / cluster-api-provider-kubevirt

Cluster API Provider for KubeVirt
Apache License 2.0
108 stars 61 forks source link

Add sentinel bootstrap file check modes in CRD #239

Closed BarthV closed 1 year ago

BarthV commented 1 year ago

https://github.com/kubernetes-sigs/cluster-api-provider-kubevirt/issues/233

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4816820717


Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 0 11 0.0%
<!-- Total: 13 24 54.17% -->
Totals Coverage Status
Change from base Build 4542574610: 0.02%
Covered Lines: 959
Relevant Lines: 1826

💛 - Coveralls
agradouski commented 1 year ago

should we include this in kubevirtmachine spec explicitly as part of this PR, given that default value is "ssh" anyway:

virtualMachineBootstrapCheck:
         checkStrategy: ssh

I know we made it backward compatible, and this is not required in the template, asking more wrt potentially confusing existing users.

one advantage of making it explicit is perhaps giving users heads up about changes (current and future) in that part of logic.

@davidvossel @pjaton wdyt?

agradouski commented 1 year ago

/lgtm

pjaton commented 1 year ago

should we include this in kubevirtmachine spec explicitly as part of this PR, given that default value is "ssh" anyway:

virtualMachineBootstrapCheck:
         checkStrategy: ssh

I know we made it backward compatible, and this is not required in the template, asking more wrt potentially confusing existing users.

one advantage of making it explicit is perhaps giving users heads up about changes (current and future) in that part of logic.

@davidvossel @pjaton wdyt?

In this case, I feel that the expected behavior by most users is really about "please use the default mechanism"; so, using unset to mean default feels more intuitive to me than making this required. I do see your point about future change, though, I simply see that there would be more impact if this is made required.

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agradouski, BarthV, pjaton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-kubevirt/blob/main/OWNERS)~~ [agradouski] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment