Skip to content

Commit

Permalink
fix(STONEINTG-641): use snapshot for SEB components directly
Browse files Browse the repository at this point in the history 
* Refactor NewBindingComponents to take components
  directly from the Snapshot and convert them into
  bindingComponents by referencing them by the resource
  metadata.name instead of spec.componentName.
* This removes the need for the extra step
  of fetching Components from the cluster
* Remove the GetAllSnapshotComponents function altogether
  as it is unused after the above changes
* Remove the now unused components parameter from
  functions which were creating/updating SEBs

Signed-off-by: dirgim <[email protected]>
  • Loading branch information
@dirgim
dirgim committed Oct 24, 2023
1 parent 7ec4fb5 commit 98c0d76
Show file tree
Hide file tree
Showing 8 changed files with 27 additions and 131 deletions.
37 changes: 13 additions & 24 deletions controllers/snapshot/snapshot_adapter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,19 +206,14 @@ func (a *Adapter) EnsureCreationOfEphemeralEnvironments() (controller.OperationR
a.logger.Error(err, "Failed to get all environments.")
return controller.RequeueWithError(err)
}
components, err := a.loader.GetAllApplicationComponents(a.client, a.context, a.application)
if err != nil {
a.logger.Error(err, "Failed to get all components.")
return controller.RequeueWithError(err)
}

for _, integrationTestScenario := range *integrationTestScenarios {
integrationTestScenario := integrationTestScenario //G601
if !shouldScenarioRunInEphemeralEnv(&integrationTestScenario) {
continue
}

_, err = a.ensureEphemeralEnvironmentForScenarioExists(&integrationTestScenario, testStatuses, components, allEnvironments)
_, err = a.ensureEphemeralEnvironmentForScenarioExists(&integrationTestScenario, testStatuses, allEnvironments)
if err != nil {
return controller.RequeueWithError(fmt.Errorf("failed to ensure existence of the environment for scenario %s: %w", integrationTestScenario.Name, err))
}
Expand Down Expand Up @@ -356,19 +351,14 @@ func (a *Adapter) EnsureSnapshotEnvironmentBindingExist() (controller.OperationR
return controller.RequeueWithError(err)
}

components, err := a.loader.GetAllSnapshotComponents(a.client, a.context, a.snapshot)
if err != nil {
return controller.RequeueWithError(err)
}

for _, availableEnvironment := range *availableEnvironments {
availableEnvironment := availableEnvironment // G601
snapshotEnvironmentBinding, err := a.loader.FindExistingSnapshotEnvironmentBinding(a.client, a.context, a.application, &availableEnvironment)
if err != nil {
return controller.RequeueWithError(err)
}
if snapshotEnvironmentBinding != nil {
err = a.updateExistingSnapshotEnvironmentBindingWithSnapshot(snapshotEnvironmentBinding, a.snapshot, components)
err = a.updateExistingSnapshotEnvironmentBindingWithSnapshot(snapshotEnvironmentBinding, a.snapshot)
if err != nil {
a.logger.Error(err, "Failed to update SnapshotEnvironmentBinding",
"snapshotEnvironmentBinding.Environment", snapshotEnvironmentBinding.Spec.Environment,
Expand All @@ -385,7 +375,7 @@ func (a *Adapter) EnsureSnapshotEnvironmentBindingExist() (controller.OperationR
"snapshotEnvironmentBinding.Snapshot", snapshotEnvironmentBinding.Spec.Snapshot)

} else {
snapshotEnvironmentBinding, err = a.createSnapshotEnvironmentBindingForSnapshot(a.application, &availableEnvironment, a.snapshot, components)
snapshotEnvironmentBinding, err = a.createSnapshotEnvironmentBindingForSnapshot(a.application, &availableEnvironment, a.snapshot)
if err != nil {
a.logger.Error(err, "Failed to create SnapshotEnvironmentBinding for snapshot",
"environment", availableEnvironment.Name,
Expand Down Expand Up @@ -449,9 +439,9 @@ func (a *Adapter) createEnvironmentForScenario(integrationTestScenario *v1beta1.
}

// createEnvironmentBindingForScenario creates SnapshotEnvironmentBinding for the given test scenario and snapshot
func (a *Adapter) createEnvironmentBindingForScenario(integrationTestScenario *v1beta1.IntegrationTestScenario, environment *applicationapiv1alpha1.Environment, components *[]applicationapiv1alpha1.Component) (*applicationapiv1alpha1.SnapshotEnvironmentBinding, error) {
func (a *Adapter) createEnvironmentBindingForScenario(integrationTestScenario *v1beta1.IntegrationTestScenario, environment *applicationapiv1alpha1.Environment) (*applicationapiv1alpha1.SnapshotEnvironmentBinding, error) {
scenarioLabelAndKey := map[string]string{gitops.SnapshotTestScenarioLabel: integrationTestScenario.Name}
binding, err := a.createSnapshotEnvironmentBindingForSnapshot(a.application, environment, a.snapshot, components, scenarioLabelAndKey)
binding, err := a.createSnapshotEnvironmentBindingForSnapshot(a.application, environment, a.snapshot, scenarioLabelAndKey)
if err != nil {
a.logger.Error(err, "Failed to create SnapshotEnvironmentBinding for snapshot",
"snapshot", a.snapshot.Name,
Expand All @@ -467,7 +457,7 @@ func (a *Adapter) createEnvironmentBindingForScenario(integrationTestScenario *v
}

// ensureEphemeralEnvironmentForScenarioExists creates or return existing epehemeral environment for the given test scenario
func (a *Adapter) ensureEphemeralEnvironmentForScenarioExists(integrationTestScenario *v1beta1.IntegrationTestScenario, testStatuses *intgteststat.SnapshotIntegrationTestStatuses, components *[]applicationapiv1alpha1.Component, allEnvironments *[]applicationapiv1alpha1.Environment) (*applicationapiv1alpha1.SnapshotEnvironmentBinding, error) {
func (a *Adapter) ensureEphemeralEnvironmentForScenarioExists(integrationTestScenario *v1beta1.IntegrationTestScenario, testStatuses *intgteststat.SnapshotIntegrationTestStatuses, allEnvironments *[]applicationapiv1alpha1.Environment) (*applicationapiv1alpha1.SnapshotEnvironmentBinding, error) {

var (
environment *applicationapiv1alpha1.Environment
Expand Down Expand Up @@ -499,7 +489,7 @@ func (a *Adapter) ensureEphemeralEnvironmentForScenarioExists(integrationTestSce
}
if binding == nil {
// create binding and add scenario name to label of binding
binding, err = a.createEnvironmentBindingForScenario(integrationTestScenario, environment, components)
binding, err = a.createEnvironmentBindingForScenario(integrationTestScenario, environment)
if err != nil {
testStatuses.UpdateTestStatusIfChanged(
integrationTestScenario.Name,
Expand Down Expand Up @@ -649,17 +639,17 @@ func (a *Adapter) createIntegrationPipelineRun(application *applicationapiv1alph
}

// createSnapshotEnvironmentBindingForSnapshot creates and returns a new snapshotEnvironmentBinding
// for the given application, environment, snapshot, and components.
// for the given application, environment, and snapshot.
// If it's not possible to create it and set the application as the owner, an error will be returned
func (a *Adapter) createSnapshotEnvironmentBindingForSnapshot(application *applicationapiv1alpha1.Application,
environment *applicationapiv1alpha1.Environment, snapshot *applicationapiv1alpha1.Snapshot,
components *[]applicationapiv1alpha1.Component, optionalLabelKeysAndValues ...map[string]string) (*applicationapiv1alpha1.SnapshotEnvironmentBinding, error) {
optionalLabelKeysAndValues ...map[string]string) (*applicationapiv1alpha1.SnapshotEnvironmentBinding, error) {
bindingName := application.Name + "-" + environment.Name + "-" + "binding"

snapshotEnvironmentBinding := gitops.NewSnapshotEnvironmentBinding(
bindingName, application.Namespace, application.Name,
environment.Name,
snapshot, *components)
snapshot)

// The SEBs for ephemeral Environments are expected to be cleaned up along with the Environment,
// while all other SEBs are meant to persist and be deleted with the Application
Expand Down Expand Up @@ -694,15 +684,14 @@ func (a *Adapter) createSnapshotEnvironmentBindingForSnapshot(application *appli
}

// updateExistingSnapshotEnvironmentBindingWithSnapshot updates and returns snapshotEnvironmentBinding
// with the given snapshot and components. If it's not possible to patch, an error will be returned.
// with the given snapshot. If it's not possible to patch, an error will be returned.
func (a *Adapter) updateExistingSnapshotEnvironmentBindingWithSnapshot(snapshotEnvironmentBinding *applicationapiv1alpha1.SnapshotEnvironmentBinding,
snapshot *applicationapiv1alpha1.Snapshot,
components *[]applicationapiv1alpha1.Component) error {
snapshot *applicationapiv1alpha1.Snapshot) error {

patch := client.MergeFrom(snapshotEnvironmentBinding.DeepCopy())

snapshotEnvironmentBinding.Spec.Snapshot = snapshot.Name
snapshotComponents := gitops.NewBindingComponents(*components)
snapshotComponents := gitops.NewBindingComponents(snapshot)
snapshotEnvironmentBinding.Spec.Components = *snapshotComponents

err := a.client.Patch(a.context, snapshotEnvironmentBinding, patch)
Expand Down
17 changes: 7 additions & 10 deletions controllers/snapshot/snapshot_adapter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -836,22 +836,18 @@ var _ = Describe("Snapshot Adapter", Ordered, func() {
var snapshotEnvironmentBinding *applicationapiv1alpha1.SnapshotEnvironmentBinding

// create snapshot environment
components := []applicationapiv1alpha1.Component{
*hasComp,
}
newLabels := map[string]string{}
newLabels[gitops.SnapshotTestScenarioLabel] = integrationTestScenario.Name
snapshotEnvironmentBinding, err := adapter.createSnapshotEnvironmentBindingForSnapshot(adapter.application, env, hasSnapshot, &components, newLabels)
snapshotEnvironmentBinding, err := adapter.createSnapshotEnvironmentBindingForSnapshot(adapter.application, env, hasSnapshot, newLabels)
Expect(err).To(BeNil())
Expect(snapshotEnvironmentBinding).NotTo(BeNil())
Expect(snapshotEnvironmentBinding.Labels[gitops.SnapshotTestScenarioLabel]).To(Equal(integrationTestScenario.Name))

// update snapshot environment with new component
componentsUpdate := []applicationapiv1alpha1.Component{
*secondComp,
}
otherSnapshot := hasSnapshot.DeepCopy()
otherSnapshot.Name = "other-snapshot"
otherSnapshot.Spec.Components = []applicationapiv1alpha1.SnapshotComponent{{Name: secondComp.Name, ContainerImage: sample_image}}

err = adapter.updateExistingSnapshotEnvironmentBindingWithSnapshot(snapshotEnvironmentBinding, hasSnapshot, &componentsUpdate)
err = adapter.updateExistingSnapshotEnvironmentBindingWithSnapshot(snapshotEnvironmentBinding, otherSnapshot)
Expect(err).To(BeNil())

// get fresh copy to make sure that SEB was updated in k8s
Expand All @@ -863,7 +859,8 @@ var _ = Describe("Snapshot Adapter", Ordered, func() {
}, updatedSEB)
return err == nil && len(updatedSEB.Spec.Components) == 1
}, time.Second*10).Should(BeTrue())
Expect(updatedSEB.Spec.Components[0].Name == secondComp.Spec.ComponentName)
Expect(updatedSEB.Spec.Snapshot == otherSnapshot.Name)
Expect(updatedSEB.Spec.Components[0].Name == secondComp.Name)

err = k8sClient.Delete(ctx, snapshotEnvironmentBinding)
Expect(err == nil || errors.IsNotFound(err)).To(BeTrue())
Expand Down
10 changes: 5 additions & 5 deletions gitops/binding.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ const (
)

// NewSnapshotEnvironmentBinding creates a new SnapshotEnvironmentBinding using the provided info.
func NewSnapshotEnvironmentBinding(bindingName string, namespace string, applicationName string, environmentName string, snapshot *applicationapiv1alpha1.Snapshot, components []applicationapiv1alpha1.Component) *applicationapiv1alpha1.SnapshotEnvironmentBinding {
bindingComponents := NewBindingComponents(components)
func NewSnapshotEnvironmentBinding(bindingName string, namespace string, applicationName string, environmentName string, snapshot *applicationapiv1alpha1.Snapshot) *applicationapiv1alpha1.SnapshotEnvironmentBinding {
bindingComponents := NewBindingComponents(snapshot)

snapshotEnvironmentBinding := &applicationapiv1alpha1.SnapshotEnvironmentBinding{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -58,11 +58,11 @@ func NewSnapshotEnvironmentBinding(bindingName string, namespace string, applica

// NewBindingComponents gets all components from the Snapshot and formats them to be used in the
// SnapshotEnvironmentBinding as BindingComponents.
func NewBindingComponents(components []applicationapiv1alpha1.Component) *[]applicationapiv1alpha1.BindingComponent {
func NewBindingComponents(snapshot *applicationapiv1alpha1.Snapshot) *[]applicationapiv1alpha1.BindingComponent {
bindingComponents := []applicationapiv1alpha1.BindingComponent{}
for _, component := range components {
for _, component := range snapshot.Spec.Components {
bindingComponents = append(bindingComponents, applicationapiv1alpha1.BindingComponent{
Name: component.Spec.ComponentName,
Name: component.Name,
})
}
return &bindingComponents
Expand Down
4 changes: 2 additions & 2 deletions gitops/binding_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,13 @@ var _ = Describe("Gitops functions for managing Snapshots", Ordered, func() {
It("ensures Binding Component is created", func() {
gitops.MarkSnapshotAsPassed(k8sClient, ctx, hasSnapshot, "test passed")
Expect(gitops.HaveAppStudioTestsSucceeded(hasSnapshot)).To(BeTrue())
bindingComponents := gitops.NewBindingComponents([]applicationapiv1alpha1.Component{*hasComp})
bindingComponents := gitops.NewBindingComponents(hasSnapshot)
Expect(bindingComponents).NotTo(BeNil())
Expect(hasComp.Spec.Replicas).To(Equal(0))
})

It("ensures a new SnapshotEnvironmentBinding is created", func() {
newSnapshotEnvironmentBinding := gitops.NewSnapshotEnvironmentBinding("sample", namespace, hasApp.Name, env.Name, hasSnapshot, []applicationapiv1alpha1.Component{*hasComp})
newSnapshotEnvironmentBinding := gitops.NewSnapshotEnvironmentBinding("sample", namespace, hasApp.Name, env.Name, hasSnapshot)
Expect(newSnapshotEnvironmentBinding).NotTo(BeNil())
Expect(newSnapshotEnvironmentBinding.Spec.Snapshot).To(Equal(hasSnapshot.Name))
Expect(newSnapshotEnvironmentBinding.Spec.Environment).To(Equal(env.Name))
Expand Down
22 changes: 0 additions & 22 deletions loader/loader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ type ObjectLoader interface {
GetAllEnvironments(c client.Client, ctx context.Context, application *applicationapiv1alpha1.Application) (*[]applicationapiv1alpha1.Environment, error)
GetReleasesWithSnapshot(c client.Client, ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) (*[]releasev1alpha1.Release, error)
GetAllApplicationComponents(c client.Client, ctx context.Context, application *applicationapiv1alpha1.Application) (*[]applicationapiv1alpha1.Component, error)
GetAllSnapshotComponents(c client.Client, ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) (*[]applicationapiv1alpha1.Component, error)
GetApplicationFromSnapshot(c client.Client, ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) (*applicationapiv1alpha1.Application, error)
GetComponentFromSnapshot(c client.Client, ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) (*applicationapiv1alpha1.Component, error)
GetComponentFromPipelineRun(c client.Client, ctx context.Context, pipelineRun *tektonv1beta1.PipelineRun) (*applicationapiv1alpha1.Component, error)
Expand Down Expand Up @@ -121,27 +120,6 @@ func (l *loader) GetAllApplicationComponents(c client.Client, ctx context.Contex
return &applicationComponents.Items, nil
}

// GetAllSnapshotComponents loads from the cluster all Components associated with the given Snapshot.
// If the Snapshot doesn't have any Components or this is not found in the cluster, an error will be returned.
func (l *loader) GetAllSnapshotComponents(c client.Client, ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) (*[]applicationapiv1alpha1.Component, error) {
components := []applicationapiv1alpha1.Component{}

for _, sc := range snapshot.Spec.Components {
component := &applicationapiv1alpha1.Component{}
err := c.Get(ctx, types.NamespacedName{
Namespace: snapshot.Namespace,
Name: sc.Name,
}, component)
if err != nil {
return nil, err
}

components = append(components, *component)
}

return &components, nil
}

// GetApplicationFromSnapshot loads from the cluster the Application referenced in the given Snapshot.
// If the Snapshot doesn't specify an Component or this is not found in the cluster, an error will be returned.
func (l *loader) GetApplicationFromSnapshot(c client.Client, ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) (*applicationapiv1alpha1.Application, error) {
Expand Down
9 changes: 0 additions & 9 deletions loader/loader_mock.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,6 @@ func (l *mockLoader) GetAllApplicationComponents(c client.Client, ctx context.Co
return &components, err
}

// GetAllSnapshotComponents returns the resource and error passed as values of the context.
func (l *mockLoader) GetAllSnapshotComponents(c client.Client, ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) (*[]applicationapiv1alpha1.Component, error) {
if ctx.Value(SnapshotComponentsContextKey) == nil {
return l.loader.GetAllSnapshotComponents(c, ctx, snapshot)
}
components, err := getMockedResourceAndErrorFromContext(ctx, SnapshotComponentsContextKey, []applicationapiv1alpha1.Component{})
return &components, err
}

// GetApplicationFromSnapshot returns the resource and error passed as values of the context.
func (l *mockLoader) GetApplicationFromSnapshot(c client.Client, ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) (*applicationapiv1alpha1.Application, error) {
if ctx.Value(ApplicationContextKey) == nil {
Expand Down
15 changes: 0 additions & 15 deletions loader/loader_mock_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,21 +139,6 @@ var _ = Describe("Release Adapter", Ordered, func() {
})
})

Context("When calling GetAllSnapshotComponents", func() {
It("returns resource and error from the context", func() {
snapshotComponents := []applicationapiv1alpha1.Component{}
mockContext := GetMockedContext(ctx, []MockData{
{
ContextKey: SnapshotComponentsContextKey,
Resource: snapshotComponents,
},
})
resource, err := loader.GetAllSnapshotComponents(nil, mockContext, nil)
Expect(resource).To(Equal(&snapshotComponents))
Expect(err).To(BeNil())
})
})

Context("When calling GetApplicationFromSnapshot", func() {
It("returns resource and error from the context", func() {
application := &applicationapiv1alpha1.Application{}
Expand Down
44 changes: 0 additions & 44 deletions loader/loader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,50 +443,6 @@ var _ = Describe("Loader", Ordered, func() {
Expect(applicationComponents).NotTo(BeNil())
})

It("ensures the Snapshot Components can be found ", func() {
snapshotComponents, err := loader.GetAllSnapshotComponents(k8sClient, ctx, hasSnapshot)
Expect(err).To(BeNil())
Expect(snapshotComponents).NotTo(BeNil())
})

It("ensures GetAllSnapshotComponents does not gather non-snapshot application components ", func() {
// otherComp has the same namespace and application as the snapshot,
// but does not belong to the snapshot itself. When creating SEBs
// for older snapshots we do not want to accidentally add components
// for newer snapshots.
otherComp := &applicationapiv1alpha1.Component{
ObjectMeta: metav1.ObjectMeta{
Name: "other-component",
Namespace: "default",
},
Spec: applicationapiv1alpha1.ComponentSpec{
ComponentName: "other-component",
Application: applicationName,
ContainerImage: "",
Source: applicationapiv1alpha1.ComponentSource{
ComponentSourceUnion: applicationapiv1alpha1.ComponentSourceUnion{
GitSource: &applicationapiv1alpha1.GitSource{
URL: SampleRepoLink,
},
},
},
},
Status: applicationapiv1alpha1.ComponentStatus{
LastBuiltCommit: "",
},
}
Expect(k8sClient.Create(ctx, otherComp)).Should(Succeed())
defer k8sClient.Delete(ctx, otherComp)

snapshotComponents, err := loader.GetAllSnapshotComponents(k8sClient, ctx, hasSnapshot)
Expect(err).To(BeNil())
Expect(snapshotComponents).NotTo(BeNil())
Expect(*snapshotComponents).To(HaveLen(1))
for _, comp := range *snapshotComponents {
Expect(comp.Name).NotTo(Equal("other-component"))
}
})

It("ensures we can get an Application from a Snapshot ", func() {
app, err := loader.GetApplicationFromSnapshot(k8sClient, ctx, hasSnapshot)
Expect(err).To(BeNil())
Expand Down

0 comments on commit 98c0d76

Please sign in to comment.