Skip to content
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

Add params.env file for opendatahub-operator install support, fixes #37 #38

Merged
merged 6 commits into from
Dec 5, 2023

Conversation

dhirajsb
Copy link
Contributor

@dhirajsb dhirajsb commented Dec 5, 2023

Description

Add a params.env file and config map generator to load the properties, and kustomize replacement to inject the properties in operator deployment.

How Has This Been Tested?

Tested by deploying locally and verifying config changes are copied from params.env properties.

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • 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

@dhirajsb dhirajsb requested review from lampajr and tarilabs December 5, 2023 04:53
lampajr
lampajr previously approved these changes Dec 5, 2023
@lampajr lampajr mentioned this pull request Dec 5, 2023
3 tasks
tarilabs
tarilabs previously approved these changes Dec 5, 2023
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one question; beside that, thank you looks good to me!

@@ -1,5 +1,5 @@
# Adds namespace to all resources.
namespace: model-registry-operator-system
namespace: opendatahub
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note the operator will move into the ODH namespace operator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we have to align it with other odh components, which all use the same namespace.
But, I'll investigate whether we can keep the current kubebuilder default config and use an overlay instead for odh. That way the operator can be used without having to do things the odh way.

@@ -70,7 +70,7 @@ spec:
- /manager
args:
- --leader-elect
image: quay.io/opendatahub/model-registry-operator:latest
image: controller
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering however if the images are placed in env file, what is the strategy for the config/manager/kustomization.yaml file, please?
I mean wouldn't that operate some changes which now should pertain to the env file?
(open question)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved odh config to a separate overlay so the default kustomize image override will still work.

@tarilabs
Copy link
Member

tarilabs commented Dec 5, 2023

If possible, I'd hold to merge this after Wednesday, December 6 h17:00 UTC so to give time for the demo to finalize using the current quay image.

@dhirajsb dhirajsb dismissed stale reviews from tarilabs and lampajr via 390c6de December 5, 2023 19:14
@dhirajsb
Copy link
Contributor Author

dhirajsb commented Dec 5, 2023

It's ok to merge this now since it's an entirely separate overlay and doesn't affect default operator deployment.

@dhirajsb dhirajsb merged commit f12777e into opendatahub-io:main Dec 5, 2023
2 checks passed
@dhirajsb dhirajsb deleted the image-params branch December 5, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants