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.

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

feat(catalog): add catalog status block updates #419

Merged
merged 16 commits into from
Aug 14, 2018

Conversation

njhale
Copy link
Member

@njhale njhale commented Aug 14, 2018

Description

Enables the status subresource for catalog sources.

@njhale njhale requested review from ecordell and alecmerdler August 14, 2018 00:20
@njhale njhale changed the title Catalog status feat(catalog): add catalog status block updates Aug 14, 2018
@@ -129,6 +129,10 @@ func TestCreateInstallPlanManualApproval(t *testing.T) {
},
}

// Attempt to get the catalog source before creating install plan
_, err = fetchCatalogSource(t, crc, ocsConfigMap, testNamespace, catalogSourceSynced)
Copy link
Member

Choose a reason for hiding this comment

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

after these changes, the test installplan didn't have a source and sourceNamespace in the spec anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like the source and sourceNamespace are not in master either.

}

// Create a new in-mem registry
src, err := registry.NewInMemoryFromConfigMap(o.OpClient, out.GetNamespace(), out.Spec.ConfigMap)
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice if we only parsed this into memory when it's changed

return builder.String()
}

func cleanupOLM(t *testing.T, namespace string) {
Copy link
Member

@ecordell ecordell Aug 14, 2018

Choose a reason for hiding this comment

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

the frustrating thing about using this everywhere is that it clears out the state of the test when you defer a call to it. it sort of defeats the purpose of keeping the test namespace around when the tests fail (there's nothing to inspect).

can we check for when the test has failed and only clear things out if it's succeeded?

It's probably just

func cleanupOLM(t *testing.T, namespace string) {
  if t.Failed {
    t.Log("skipping cleanup")
    return
  }
//...

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes total sense.

- adds deferred cleanup of all OLM resources
- adds skip cleanup global field to avoid cleanup when any test fails
}

// Check for config map changes
if catsrc.Status.ConfigMapResource == nil || catsrc.Status.ConfigMapResource.Name != configMap.GetName() || catsrc.Status.ConfigMapResource.ResourceVersion != configMap.GetResourceVersion() {
Copy link
Member

Choose a reason for hiding this comment

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

check the opposite condition here and return early?

@@ -4,12 +4,25 @@ import (
"errors"
"testing"

"github.com/ghodss/yaml"

Copy link
Member

Choose a reason for hiding this comment

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

nit: space

for _, step := range installPlan.Status.Plan {
if step.Resource.Kind == v1alpha1.ClusterServiceVersionKind {
if err := crc.OperatorsV1alpha1().ClusterServiceVersions(namespace).Delete(step.Resource.Name, &metav1.DeleteOptions{}); err != nil {
if err := crc.OperatorsV1alpha1().ClusterServiceVersions(namespace).Delete(step.Resource.Name, deleteOptions); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

our fetch<..> functions take a *testing.T, maybe the build<...>CleanupFunc functions should too; then you could assert and fail when cleanup fails (which might be helpful since that's been a source of errors).

@@ -28,10 +30,23 @@ import (
const (
pollInterval = 1 * time.Second
pollDuration = 5 * time.Minute

etcdVersion = "3.2.13"
prometheusVersion = "v1.7.0"
Copy link
Member

Choose a reason for hiding this comment

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

2.3.2 :)

Copy link
Member

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

LGTM when the CI passes

@njhale njhale merged commit a8b576c into operator-framework:master Aug 14, 2018
@njhale njhale deleted the catalog-status branch August 15, 2018 22:55
njhale added a commit to njhale/operator-lifecycle-manager that referenced this pull request Sep 10, 2018
feat(catalog): add catalog status block updates
ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019
feat(catalog): add catalog status block updates
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.

2 participants