Read env variables from KfDef Parameters

[VaishnaviHire]

Fixes #179

How Has This Been Tested?

1. Install Open Data Hub operator
2. Update CSV with following two changes:
   • spec.image : quay.io/vhire/opendatahub-operator:env
   • Add following env variable under spec.env

     name: MINIMAL_IMAGE
     value: quay.io/thoth-station/s2i-minimal-py38-notebook:v0.3.0
     

3. Deploy following KfDef

apiVersion: kfdef.apps.kubeflow.org/v1
kind: KfDef
metadata:
  name: opendatahub
  namespace: opendatahub
spec:
  applications:
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: odh-common
    name: odh-common
  - kustomizeConfig:
      overlays:
      - additional
      parameters:
       - name: minimal-image
         value: $MINIMAL_IMAGE
      repoRef:
        name: manifests
        path: jupyterhub/notebook-images
    name: notebook-images
  repos:
  - name: manifests
    uri: https://github.com/VaishnaviHire/odh-manifests/tarball/test_image_vars

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

[samuelvl]

/cc

[samuelvl]

It works fine if I define the parameters as following:

parameters:
  - name: minimal-image
    value: $MINIMAL_IMAGE

Or

parameters:
  - name: minimal-image
    value: quay.io/modh/odh-minimal-notebook-container:v1.18.0-2

However when using ${VAR} it doesn't work:

parameters:
  - name: minimal-image
    value: ${MINIMAL_IMAGE}

I see the following error in the operator logs:

time="2022-10-19T11:28:35Z" level=warning msg="Encountered error applying application notebook-images:  (kubeflow.error): Code 500 with message: Apply.Run : ImageStream.image.openshift.io \"s2i-minimal-notebook\" is invalid: [[]: Internal error: imagestreams \"s2i-minimal-notebook\" is invalid: spec.tags[v0.3.0-py38].from.name: Invalid value: \"\": must be of the form <tag>, <repo>:<tag>, <id>, or <repo>@<id>, spec.tags[v0.3.0-py38].from.name: Required value]"
time="2022-10-19T11:28:35Z" level=warning msg="Will retry in 4 seconds."

I'm missing some unit tests to cover the getParameterValue function.

[VaishnaviHire]

It works fine if I define the parameters as following:

parameters:
  - name: minimal-image
    value: $MINIMAL_IMAGE

Or

parameters:
  - name: minimal-image
    value: quay.io/modh/odh-minimal-notebook-container:v1.18.0-2

However when using ${VAR} it doesn't work:

parameters:
  - name: minimal-image
    value: ${MINIMAL_IMAGE}

I see the following error in the operator logs:

time="2022-10-19T11:28:35Z" level=warning msg="Encountered error applying application notebook-images:  (kubeflow.error): Code 500 with message: Apply.Run : ImageStream.image.openshift.io \"s2i-minimal-notebook\" is invalid: [[]: Internal error: imagestreams \"s2i-minimal-notebook\" is invalid: spec.tags[v0.3.0-py38].from.name: Invalid value: \"\": must be of the form <tag>, <repo>:<tag>, <id>, or <repo>@<id>, spec.tags[v0.3.0-py38].from.name: Required value]"
time="2022-10-19T11:28:35Z" level=warning msg="Will retry in 4 seconds."

I'm missing some unit tests to cover the getParameterValue function.

I think I misread the supported formats. Updating the regex.

[Jooho]

@VaishnaviHire keep in mind when the value is specified like the following:

parameters:
  - name: minimal-image
    value: ${MINIMAL_IMAGE}

In the kfdef object, it will be changed like the following

parameters:
  - name: minimal-image
    value: '${MINIMAL_IMAGE}'

I am not sure your code will be impacted by this or not but I just want you to head up this kind openshift manifest behavior.

[VaishnaviHire]

@VaishnaviHire keep in mind when the value is specified like the following:

parameters:
  - name: minimal-image
    value: ${MINIMAL_IMAGE}

In the kfdef object, it will be changed like the following

parameters:
  - name: minimal-image
    value: '${MINIMAL_IMAGE}'

I am not sure your code will be impacted by this or not but I just want you to head up this kind openshift manifest behavior.

Thanks ! This should not impact the code.

[VaishnaviHire]

@samuelvl Addressed the comments and updated the operator image

To run unit tests, use the following command

go test -run  TestGetParameterValue ./ -v

[samuelvl]

It works great now! Only one question, is this return value really needed?

Suggested change

	}else {
	return value
	}
	}

Running the coverage of this function I found that if I remove it we can achieve 100%

# With return value
$ go test -run TestGetParameterValue -v -covermode=count -coverprofile coverage ./pkg/kfapp/kustomize/...
$ go tool cover -func=coverage
github.com/kubeflow/kfctl/v3/pkg/kfapp/kustomize/kustomize.go:1467:	getParameterValue			90.9%

# After applying the suggestion
$ go test -run TestGetParameterValue -v -covermode=count -coverprofile coverage ./pkg/kfapp/kustomize/...
$ go tool cover -func=coverage
github.com/kubeflow/kfctl/v3/pkg/kfapp/kustomize/kustomize.go:1467:	getParameterValue			100.0%

[VaishnaviHire]

It's no longer required. Removed it. Thanks !

[samuelvl]

/lgtm

[Jooho]

/lgtm

[samuelvl]

@LaVLaS Could you approve this PR? :)

[VaishnaviHire]

@LaVLaS Did you get a chance to review this ? I think this can be merged for 1.4.1

[LaVLaS]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LaVLaS, samuelvl

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 [LaVLaS,samuelvl]

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