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

Put all resources in specified provider namespace #538

Merged
merged 4 commits into from
Apr 23, 2019

Conversation

lblackstone
Copy link
Member

This is an updated version of #506 that should correctly handle CRs.

Fixes #415

@lblackstone
Copy link
Member Author

Error is:

kubernetes:config.istio.io:rule istio-system/promhttp  error: kubernetes:config.istio.io/v1alpha2:rule resource 'istio-system/promhttp' has a problem:  Found API Group, but it did not contain a schema for "config.istio.io/v1alpha2, Kind=rule"

Not sure yet if that's legit, or if https://github.com/pulumi/pulumi-kubernetes/pull/538/files#diff-6155820fac2dd2fda7353505b1f69319R307 is invalid.

Copy link
Contributor

@metral metral left a comment

Choose a reason for hiding this comment

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

overall LGTM. It'd be great to get a description somewhere of the logic being done here w.r.t how we're:

  • handling CR / CRDs
  • the need for overrideNamespace - following the code it makes sense, but its hard to reason about which namespace we're using / not using

pkg/clients/clients.go Outdated Show resolved Hide resolved
@lblackstone
Copy link
Member Author

@metral I think I addressed all your feedback. Added a note to the code clarifying the override behavior.

CRs are a special case because it isn't always possible to tell if they are Namespaced/Global scoped during preview. As noted in the code, this could result in a namespace being set erroneously on a Cluster-scoped CR for previews, but this would be corrected during the Create step, when the required information is available. This correction will be reflected in further updates/diffs as well, so even if the erroneous namespace on the resource changes, it will not require a replacement.

@lblackstone
Copy link
Member Author

@hausdorff Have you had a chance to look this over? The main change from the previously-merged #506 is the improved logic related to unregistered CRs. See the explanation in my comment to Mike for details.

Copy link
Contributor

@metral metral left a comment

Choose a reason for hiding this comment

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

lgtm

@lblackstone lblackstone merged commit e1beb1e into master Apr 23, 2019
@pulumi-bot pulumi-bot deleted the lblackstone/provider-ns branch April 23, 2019 18:09
hausdorff added a commit that referenced this pull request Jul 13, 2019
hausdorff added a commit that referenced this pull request Jul 13, 2019
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.

Provider namespace field doesn't put new resources in the namespace
2 participants