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

Remove irrelevant external metrics APIService resources #277

Conversation

ChristianZaccaria
Copy link
Contributor

Issue link

Closes #247

What changes have been made

As mentioned in the issue: The multi-cluster mode provided by MCAD is experimental and is not used. Even if the multi-cluster mode is deactivated by default, a number of resources are installed only for the purpose of its operation.

For this reason, we are removing the APIService resources and ClusterRoleBindings that are irrelevant to the single cluster setup.

The removal of the CRBs was done as part of this PR #270

Checks

  • e2e tests should continue to pass.

@@ -22,7 +22,6 @@ import (

var multiClusterAppDispatcherTemplates = []string{
"mcad/configmap.yaml.tmpl",
"mcad/service.yaml.tmpl",
"mcad/serviceaccount.yaml.tmpl",
"mcad/deployment.yaml.tmpl",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the corresponding ports from the Deployment can also be removed:

ports:
  - name: https
     containerPort: 6443
     protocol: TCP
  - name: http
     containerPort: 8080
    protocol: TCP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! Changes were made now.

I do have a few thoughts though. Should we also be removing items from the MCAD test data ?

I.e., in mcad_test_results, there are some rolebindings and the same mentioned ports in the deployment of these tests.

These tests seem to run on the tag-and-build workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch too! Indeed, these test data files should be amended accordingly too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent, the respective files have been removed. Waiting on tests to pass.

@astefanutti
Copy link
Contributor

/lgtm

@astefanutti
Copy link
Contributor

/approve

@openshift-ci
Copy link

openshift-ci bot commented Sep 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti

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-ci openshift-ci bot added the approved label Sep 7, 2023
@openshift-merge-robot openshift-merge-robot merged commit 8e47723 into project-codeflare:main Sep 7, 2023
9 checks passed
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.

Deploy MCAD in agent mode only (without multi-cluster support)
3 participants