Add support for custom subscription config #446

VaishnaviHire · 2023-08-14T18:49:19Z

Fixes #436

Description

How Has This Been Tested?

Deploy following dev catalogsource

quay.io/vhire/opendatahub-operator-catalog:v2.0.32

Subscribe to operator using following subscription

apiVersion: operators.coreos.com/v1alpha1
kind: Subscription
metadata:
  name: opendatahub-operator
  namespace: openshift-operators
spec:
  channel: fast
  name: opendatahub-operator
  source: opendatahub-dev-catalog
  sourceNamespace: openshift-marketplace
  config:
     env:
        - name: "DISABLE_DSC_CONFIG"

Verify no namespaces are created
Verify when DataScienceCluster CR is created, it goes into Error phase

To Enable Components
5. Create DSCInitialization CR

apiVersion: dscinitialization.opendatahub.io/v1alpha1
kind: DSCInitialization
metadata:
  name: default
spec:
  monitoring:
    enabled: false
    namespace: 'opendatahub'
  applicationsNamespace: 'opendatahub'

Then create DataScienceCluster CR

apiVersion: datasciencecluster.opendatahub.io/v1alpha1
kind: DataScienceCluster
metadata:
  name: default
spec:
  components:
    dashboard:
      enabled: true

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

main.go

etirelli · 2023-08-14T20:27:58Z

main.go

-			Monitoring: dsci.Monitoring{
-				Enabled: false,
+	// Check if Auto configuration is enabled
+	if os.Getenv("ENABLE_AUTO_CONFIG") == "" || os.Getenv("ENABLE_AUTO_CONFIG") != "false" {


I wonder if it wouldn't be better to check == "true" for proceeding, == "false" for disabling and failing if it is not true or false? it would make intent explicit and avoid silent failures on this critical flag?

When a user installs the operator through OperatorHub, subscription is created for the user by default. That will not include this config. In that case, if we set up failure for not defining the flag, it will result in operator install failure

Sorry, what I mean is if the user makes a typo like: ENABLE_AUTO_CONFIG=flse . With the current code, although the user intended to disable auto-config, it will silently proceed with the configuration.
I completely agree with you that in the absence of the flag, it should use the default behaviour of configuring everything.

will that be easier to just have a flag in subscritipion, rather than set to "true" or "false".
so, if install from opendatahub, this flag (say, ENABLE_AUTO_CONFIG) does not exist. then do as-is
if subsciption created by user and set this flag ENABLE_AUTO_CONFIG then proceed without create DSCI instance.

.spec.config.env Description Env is a list of environment variables to set in the container. Cannot be updated.

instead of using

.spec.config.env[]

btw, can we change the name to something more specific,
the final catalogsource image can have different operators in it.
if others have a logic check on env ENABLE_AUTO_CONFIG as well.

etirelli

Added a comment, but I think we can merge as address that later.

LGTM

zdtsw · 2023-08-15T05:29:42Z

main.go

-			Monitoring: dsci.Monitoring{
-				Enabled: false,
+	// Check if Auto configuration is enabled
+	if os.Getenv("ENABLE_AUTO_CONFIG") == "" || os.Getenv("ENABLE_AUTO_CONFIG") != "false" {


will that be easier to just have a flag in subscritipion, rather than set to "true" or "false".
so, if install from opendatahub, this flag (say, ENABLE_AUTO_CONFIG) does not exist. then do as-is
if subsciption created by user and set this flag ENABLE_AUTO_CONFIG then proceed without create DSCI instance.

.spec.config.env Description Env is a list of environment variables to set in the container. Cannot be updated.

instead of using

.spec.config.env[]

zdtsw · 2023-08-15T05:32:07Z

main.go

-			Monitoring: dsci.Monitoring{
-				Enabled: false,
+	// Check if Auto configuration is enabled
+	if os.Getenv("ENABLE_AUTO_CONFIG") == "" || os.Getenv("ENABLE_AUTO_CONFIG") != "false" {


btw, can we change the name to something more specific,
the final catalogsource image can have different operators in it.
if others have a logic check on env ENABLE_AUTO_CONFIG as well.

controllers/datasciencecluster/datasciencecluster_controller.go

zdtsw

/lgtm

openshift-ci · 2023-08-15T13:21:15Z

[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:

~~OWNERS~~ [etirelli]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

VaishnaviHire requested review from etirelli and zdtsw August 14, 2023 18:49

openshift-ci bot requested a review from cfchase August 14, 2023 18:49

etirelli reviewed Aug 14, 2023

View reviewed changes

main.go Outdated Show resolved Hide resolved

etirelli reviewed Aug 14, 2023

View reviewed changes

VaishnaviHire force-pushed the add_subscription_flag branch from 1c94bae to e8bb313 Compare August 14, 2023 22:25

etirelli approved these changes Aug 14, 2023

View reviewed changes

openshift-ci bot assigned etirelli Aug 14, 2023

openshift-ci bot added lgtm approved labels Aug 14, 2023

zdtsw reviewed Aug 15, 2023

View reviewed changes

Add support for custom subscription config

d97ced0

VaishnaviHire force-pushed the add_subscription_flag branch from e8bb313 to d97ced0 Compare August 15, 2023 13:13

openshift-ci bot removed the lgtm label Aug 15, 2023

zdtsw approved these changes Aug 15, 2023

View reviewed changes

openshift-ci bot assigned zdtsw Aug 15, 2023

openshift-ci bot added the lgtm label Aug 15, 2023

VaishnaviHire merged commit 94be749 into opendatahub-io:main Aug 15, 2023

zdtsw mentioned this pull request Sep 28, 2023

Add check on DSCI intance to explicity on "default" as name #575

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom subscription config #446

Add support for custom subscription config #446

VaishnaviHire commented Aug 14, 2023 •

edited

Loading

etirelli Aug 14, 2023

VaishnaviHire Aug 14, 2023

etirelli Aug 14, 2023 •

edited

Loading

zdtsw Aug 15, 2023

zdtsw Aug 15, 2023

VaishnaviHire Aug 15, 2023

etirelli left a comment

zdtsw Aug 15, 2023

zdtsw Aug 15, 2023

zdtsw left a comment

openshift-ci bot commented Aug 15, 2023

Add support for custom subscription config #446

Add support for custom subscription config #446

Conversation

VaishnaviHire commented Aug 14, 2023 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

etirelli Aug 14, 2023

Choose a reason for hiding this comment

VaishnaviHire Aug 14, 2023

Choose a reason for hiding this comment

etirelli Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

zdtsw Aug 15, 2023

Choose a reason for hiding this comment

zdtsw Aug 15, 2023

Choose a reason for hiding this comment

VaishnaviHire Aug 15, 2023

Choose a reason for hiding this comment

etirelli left a comment

Choose a reason for hiding this comment

zdtsw Aug 15, 2023

Choose a reason for hiding this comment

zdtsw Aug 15, 2023

Choose a reason for hiding this comment

zdtsw left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 15, 2023

VaishnaviHire commented Aug 14, 2023 •

edited

Loading

etirelli Aug 14, 2023 •

edited

Loading