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

Implement DSCInitialization Controller #249

Conversation

VaishnaviHire
Copy link
Member

@VaishnaviHire VaishnaviHire commented Jun 14, 2023

Introduces changes defined in #245

Description

  • Ability to create Namespaces
  • Ability to create namespace specific Network Policies, RoleBindings
  • Identifying if Addon Installation
  • Ability to add Addon specific logic
  • Optional Dev field to refer custom manifests
  • Deploy Monitoring resources (Partial implementation, paused any changes until further confirmation regarding implementation of Monitoring stack)
  • Deploy Dashboard CRDs
  • Add Logs and Events
  • Cleanup resources
  • Ability of Operator to create DSCInitialization CR
  • Add Unit Tests for DSCInitialization Controller
  • Add ConsoleLink for Managed services

How Has This Been Tested?

Test already built image

  1. Deploy Operator
 operator-sdk run bundle quay.io/vhire/opendatahub-operator-bundle:dev-0.0.1
  1. Verify default DSCInitialization instance is created
  2. Verify opendatahub namespace is created with networkpolicies and odh specific labels

Test custom image build

  1. Pull in updated manifests
make get-manifests -e MANIFEST_REPO=VaishnaviHire -e MANIFEST_RELEASE=rearch_manifests
  1. Build operator Image
 make image -e IMG=<quay.io/username/opendatahub-operator:<TAG>>
  1. Build Bundle
 make bundle-build bundle-push BUNDLE_IMG=quay.io/<username>/opendatahub-operator-bundle:<TAG>
  1. Run Bundle
operator-sdk run bundle -e BUNDLE_IMG=quay.io/<username>/opendatahub-operator-bundle:<TAG>

Issues

Fix Path for local manifest bundle

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

pkg/deploy/deploy.go Outdated Show resolved Hide resolved
pkg/deploy/deploy.go Outdated Show resolved Hide resolved
@VaishnaviHire VaishnaviHire force-pushed the add_dscinit_controller branch from 9be4aef to 46d5f6e Compare June 21, 2023 09:43
@VaishnaviHire VaishnaviHire changed the title [WIP] Implement DSCInitialization Controller Implement DSCInitialization Controller Jun 21, 2023
- bases/odhapplications.dashboard.opendatahub.io_odhapplications.yaml
- bases/odhdashboardconfigs.opendatahub.io_odhdashboardconfigs.yaml
- bases/odhdocuments.dashboard.opendatahub.io_odhdocuments.yaml
- bases/odhquickstarts.console.openshift.io_odhquickstarts.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Folder should be base not bases if you are attempting to follow the base/overlay kustomize convention.

The kustomization.yaml file should also be in the base folder by convention.

This is more personal preference, but I usually have a strong aversion from deploying anything from a base folder. At a minimum I almost always create an overlays/default folder that simply references the base. This gives you more flexibility if you later need to introduce an alternative overlay and you may need to move something from the base to the default overlay without impacting anything that is currently deploying from the default overlay.

With that being said, I'm unsure what the use case for overlays in the operator might be or when you might want to leverage an overlay strategy here so it may just make more sense to drop the bases folder entirely and put everything directly in the crd folder.

Copy link
Member Author

@VaishnaviHire VaishnaviHire Jun 21, 2023

Choose a reason for hiding this comment

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

This kustomize format is generated by the operator-sdk/kubebuilder when we run operator-sdk generate manifests.

You are correct, with respect to having default folder. For application manifests, we do check for kustomization.yaml file or default. So components are expected to have a default manifest path.

@VaishnaviHire
Copy link
Member Author

From today's sync up -

  • update the PR to add reconciliation for Network Policies
  • keep Monitoring resources configurable

Moving to wip to address above comments

@VaishnaviHire VaishnaviHire changed the title Implement DSCInitialization Controller [WIP] Implement DSCInitialization Controller Jun 21, 2023
@VaishnaviHire VaishnaviHire force-pushed the add_dscinit_controller branch from 46d5f6e to 8e4580d Compare June 23, 2023 12:53
@VaishnaviHire VaishnaviHire changed the title [WIP] Implement DSCInitialization Controller Implement DSCInitialization Controller Jun 23, 2023
@VaishnaviHire VaishnaviHire force-pushed the add_dscinit_controller branch from 8e4580d to 78c1d61 Compare June 23, 2023 20:23

instance := &dsci.DSCInitialization{}
err := r.Client.Get(ctx, req.NamespacedName, instance)
// Only apply reconcile logic to 'default' instance of DataScienceInitialization
err := r.Client.Get(ctx, types.NamespacedName{Name: "default"}, instance)
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor nitpick, maybe set a constant for "default" as it is also used in the predicate below?

Copy link
Contributor

@etirelli etirelli left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link

openshift-ci bot commented Jun 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: etirelli

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 42982ae into opendatahub-io:feature/rearchitecture Jun 27, 2023
VaishnaviHire added a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Apr 24, 2024
Update pkg/upgrade/upgrade.go



fix linter

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Co-authored-by: Vaishnavi Hire <vhire@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants