-
Notifications
You must be signed in to change notification settings - Fork 273
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(openshift): Run and Test actions #4730
Conversation
@@ -233,6 +233,11 @@ export async function getSystemNamespace( | |||
api?: KubeApi | |||
): Promise<string> { | |||
const namespace = { name: provider.config.gardenSystemNamespace } | |||
// HACK: in OpenShift, we work in only one namespace, the one assigned to the developer | |||
if (!namespace.name) { | |||
log.warn("No system namespace found, using the current namespace. If you are using OpenShift, ignore this warning.") |
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.
Is there a reason to print the message in the first place for openshift users?
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.
This is within the generic kubernetes code, and we don't necessarily know here if we are running OpenShift or not. Something to fix later when we have the OpenShfit provider in a better shape and can propagate the information around
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 think the ctx passed here has that data though
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.
Currently, we don't have a proper OpenShift provider context - it is using the KubernetesProvider and its context under the hood. Definitely something we need to improve later!
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.
LGTM, Thank you 💯
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #4626
Fixes #4627
Special notes for your reviewer: