-
Notifications
You must be signed in to change notification settings - Fork 42
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
Introduce additional requirements for the spaces install #350
Introduce additional requirements for the spaces install #350
Conversation
- providers (and related components) are required and will be installed - ingress-nginx is now required and will be installed Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a couple spots where it looks like we're creating a new context when we should be reusing an existing one. Otherwise LGTM.
if _, err := h.crdclient. | ||
CustomResourceDefinitions(). | ||
Get( | ||
context.Background(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be reusing the existing context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
if _, err := k.crdclient. | ||
CustomResourceDefinitions(). | ||
Get( | ||
context.Background(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be reusing the existing context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Description of your changes
This removes some scope creep that was occurring with the
Spaces
install.I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
newrequirements.for.spaces.install.mov