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

Add CatalogSource Namespace to Subscription Objects #380

Merged
merged 1 commit into from
Jul 16, 2018

Conversation

alecmerdler
Copy link
Member

@alecmerdler alecmerdler commented Jul 11, 2018

Description

Makes it possible to find the contents of a CatalogSource-v1 without needing permission to fetch in all namespaces.

Fixes https://jira.coreos.com/browse/ALM-639

@alecmerdler alecmerdler requested a review from ecordell July 11, 2018 15:21
// Operator represents a Kubernetes operator that executes InstallPlans by
// resolving dependencies in a catalog.
type Operator struct {
*queueinformer.Operator
client versioned.Interface
namespace string
sources map[string]registry.Source
sources map[sourceKey]registry.Source
Copy link
Member Author

@alecmerdler alecmerdler Jul 11, 2018

Choose a reason for hiding this comment

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

Use complex struct which contains both name and namespace as map key (see https://play.golang.org/p/ngwKvwPggu5).

Copy link
Member

Choose a reason for hiding this comment

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

👍 for compound keys

@ecordell
Copy link
Member

Why don't we make this backwards compatible by saying if CatalogSourceNamespace == ""; CatalogSourceNamespace = <global catalog namespace from command line args>?

@alecmerdler
Copy link
Member Author

@ecordell That doesn't help the UI (or non-admin users) find the CatalogSource for a Subscription. We could alternatively add it to status.sourceNamespace instead, but aren't we going to run into name collisions anyway?

@alecmerdler alecmerdler force-pushed the ALM-639 branch 2 times, most recently from becf985 to 5a1140a Compare July 11, 2018 19:06
@alecmerdler
Copy link
Member Author

@ecordell Tests are passing, made it backwards compatible.

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.

looks good! couple of small nitpicks

@@ -270,14 +275,14 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error {
var notFoundErr error
for sourceName, source := range o.sources {
Copy link
Member

Choose a reason for hiding this comment

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

nit: for key, source

if notFoundErr != nil {
continue
}

// Look up the CatalogSource.
catsrc, err := o.client.CatalogsourceV1alpha1().CatalogSources(o.namespace).Get(sourceName, metav1.GetOptions{})
catsrc, err := o.client.CatalogsourceV1alpha1().CatalogSources(o.namespace).Get(sourceName.name, metav1.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

should be CatalogSources(sourceName.namespace) just in case?

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

@alecmerdler alecmerdler merged commit 2f09d32 into operator-framework:master Jul 16, 2018
@alecmerdler alecmerdler deleted the ALM-639 branch July 16, 2018 19:32
njhale pushed a commit to njhale/operator-lifecycle-manager that referenced this pull request Sep 10, 2018
Add CatalogSource Namespace to Subscription Objects
ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019
Add CatalogSource Namespace to Subscription Objects
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