-
Notifications
You must be signed in to change notification settings - Fork 135
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
Unset Tech Preview components by default #708
Unset Tech Preview components by default #708
Conversation
fmt.Printf("DataScienceCluster resource already exists. It will not be updated with default DSC.") | ||
return nil | ||
} | ||
// Do not update the DSC if it already exists |
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.
@etirelli Can you confirm for Managed service, we would not be patching with default DSC if the instance is already created?
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 am fine with the PR as long as we can confirm ^
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.
@VaishnaviHire correct, that is my understanding. If a DSC already exists, we should preserve it.
ea06557
to
2ae4caf
Compare
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
pkg/upgrade/upgrade.go
Outdated
// Set the default DSC name depending on the platform | ||
var DSCName string | ||
if platform == deploy.ManagedRhods || platform == deploy.SelfManagedRhods { | ||
DSCName = "rhods" |
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.
sorry, now I understand what you said in the meeting. I don't think this should be named "rhods". Can we keep it as "default" as well? Otherwise, we might want to use a name that is more resilient to change, like "default-dsc" or similar.
4ab9966
to
64da37c
Compare
64da37c
to
8b53059
Compare
@@ -15,7 +15,7 @@ metadata: | |||
"app.kubernetes.io/name": "datasciencecluster", | |||
"app.kubernetes.io/part-of": "opendatahub-operator" | |||
}, | |||
"name": "default" | |||
"name": "default-dsc" |
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 we update line 13 as well? but can be done in "config" first
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
pkg/upgrade/upgrade.go
Outdated
APIVersion: "dscinitialization.opendatahub.io/v1", | ||
}, | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "default-dsc", |
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.
or default-dsci ?
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
// Check if user opted for disabling DSC configuration | ||
_, disableDSCConfig := os.LookupEnv("DISABLE_DSC_CONFIG") | ||
if !disableDSCConfig { | ||
if err = upgrade.CreateDefaultDSCI(setupClient, platform, dscApplicationsNamespace, dscMonitoringNamespace); err != nil { |
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.
to call it as part of upgrade
package is a bit confusion but i understand it is calling the CreateDefaultDSCI
function
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.
Yeah I kept the function in same package as createDefaultDSC
ff13d28
to
abb9182
Compare
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
abb9182
to
7c65b16
Compare
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: etirelli, zdtsw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Unset Tech Preview components by default * Update default DSC and DSCI name (cherry picked from commit 1ccbe05)
* Unset Tech Preview components by default * Update default DSC and DSCI name (cherry picked from commit 1ccbe05)
The comment is outdated. The original patch 1ccbe05 ("Unset Tech Preview components by default (opendatahub-io#708)") had this logic, but it was removed by f756e40 ("Update incubation with downstream changes (opendatahub-io#783)") Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
Fixes #706
Description
How Has This Been Tested?
Merge criteria: