✨ Add migration from helm to boxcutter revision - #9
Conversation
63ad1ed to
4ad5221Compare| x-kubernetes-embedded-resource: true | ||
| x-kubernetes-preserve-unknown-fields: true | ||
| required: | ||
| - collisionProtection |
There was a problem hiding this comment.
If collisionProtection has a default ("Prevent"), it doesn't seem like it also makes sense to be required?
86bc6d7 to
34eafeeCompare| // +kubebuilder:validation:Enum=Active;Paused;Archived | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Paused' || oldSelf == 'Archived' && oldSelf == self", message="can not un-archive" | ||
| LifecycleState ClusterExtensionRevisionLifecycleState `json:"lifecycleState,omitempty"` | ||
| // Revision number orders changes over time, must always be previous revision +1. |
There was a problem hiding this comment.
I don't get what we're trying to say with "Revision number orders changes over time". Maybe just "Revision number changes over time"? Maybe "Revision provides historical ordering of revisions, must always be previous +1"?
| return fmt.Errorf("unable to create helm action client getter: %w", err) | ||
| } | ||
| // TODO: add support for preflight checks |
| type ClusterExtensionRevisionGenerator interface { | ||
| GenerateRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) | ||
| GenerateRevisionFromHelmRelease( |
There was a problem hiding this comment.
could we be mixing concerns here? Maybe we could use generics in the interface to define the input type? (e.g. helmRelease vs bundleFS)?
| }) | ||
| } | ||
| rev := r.buildClusterExtensionRevision(objs, ext, map[string]string{ |
There was a problem hiding this comment.
We could make this a revision generator and then just use composition for the bundle and helm release generators....wdyt?
| } | ||
| func Test_SimpleRevisionGenerator_Success(t *testing.T) { | ||
| func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) { |
There was a problem hiding this comment.
separating the Objects -> Revision, Bundle -> Revision and Chart -> Revision + composition (e.g. Objects -> Revision being used in Chart -> Revision) might also facilitate testing here since we'd only need to test the integration between the chart -> revision and objects -> revision (i.e. that we extract the correct objects from the chart and pass it down). This could help save us time later on as we iterate over the objects to revision process.
| client.AssertExpectations(t) | ||
| }) | ||
| t.Run("does not create revision when revisions exist", func(t *testing.T) { |
There was a problem hiding this comment.
could the title be wrong here? We aren't returning any revisions
| ObjectMeta: metav1.ObjectMeta{Name: "test123"}, | ||
| } | ||
| client. |
There was a problem hiding this comment.
is it possible to check that client.Client was not called?
| }, | ||
| } | ||
| // TODO: we should make constants for the ClusterExtensionRevision condition types. |
6d7efbf to
7e4f03bCompare34eafee to
8d7de9aCompareUh oh!
There was an error while loading. Please reload this page.
Description
Reviewer Checklist