Skip to content

gitops-2415 added reconcile plugin condition#398

Closed
ciiay wants to merge 2 commits into
redhat-developer:masterfrom
ciiay:gitops-2415-add-reconcile-plugin-condition
Closed

gitops-2415 added reconcile plugin condition#398
ciiay wants to merge 2 commits into
redhat-developer:masterfrom
ciiay:gitops-2415-add-reconcile-plugin-condition

Conversation

@ciiay
Copy link
Copy Markdown
Member

@ciiay ciiay commented Nov 21, 2022

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:
To avoid multiple "Environments" button showing up in left menu on Developer tab in DevConsole.

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Which issue(s) this PR fixes:

Fixes GITOPS-2415

Test acceptance criteria:

  • Unit Test
  • E2E Test

How to test changes / Special notes to the reviewer:

  1. On OCP 4.15 or greater, observe that Console Plugin section is showing up on GitOps-operator installation page. Only one "Environments" button exists on Developer tab left menu.
  2. On OCP 4.14 or lower, observe that Console Plugin section is not showing up on GitOps-operator installation page. Only one "Environments" button exists on Developer tab left menu.

@openshift-ci openshift-ci Bot added the kind/enhancement New feature or request label Nov 21, 2022
@ciiay ciiay force-pushed the gitops-2415-add-reconcile-plugin-condition branch from f33d49d to 31dd4ea Compare November 21, 2022 16:41
@ciiay ciiay requested review from keithchong and wtam2018 November 21, 2022 16:59
@jaideepr97
Copy link
Copy Markdown
Contributor

Hi @ciiay @keithchong
Are we still going with the approach of making the dynamic plugin optionally enabled (but disabled by default) under 4.15?
In this PR it seems like the plugin will not work for clusters under 4.15 at all

@ciiay
Copy link
Copy Markdown
Member Author

ciiay commented Nov 21, 2022

Hi @ciiay @keithchong Are we still going with the approach of making the dynamic plugin optionally enabled (but disabled by default) under 4.15? In this PR it seems like the plugin will not work for clusters under 4.15 at all

Hi Jaideep, thanks for the review. Yes, going forward we will only reconcile dynamic plugin resources when OCP is 4.15 or higher version. On OCP 4.14 or lower, user will still be able to use static plugin. Please refer to the GITOPS-2415 and Keith's comment in GITOPS-2369

@ciiay
Copy link
Copy Markdown
Member Author

ciiay commented Nov 21, 2022

/test v4.10-e2e

@jaideepr97
Copy link
Copy Markdown
Contributor

jaideepr97 commented Nov 21, 2022

Hi @ciiay @keithchong Are we still going with the approach of making the dynamic plugin optionally enabled (but disabled by default) under 4.15? In this PR it seems like the plugin will not work for clusters under 4.15 at all

Hi Jaideep, thanks for the review. Yes, going forward we will only reconcile dynamic plugin resources when OCP is 4.15 or higher version. On OCP 4.14 or lower, user will still be able to use static plugin. Please refer to the GITOPS-2415 and Keith's comment in GITOPS-2369

@ciiay according to that comment we will disable it by default, but users should have the option to enable it - and we should document that enabling the dynamic plugin means there will be 2 environment tabs rendered in the console

This will require changes in the CRD to add a new field to enable/disable teh dynamic plugin, and the operator would need to be able to reconcile the plugin based on that setting in the CR

@ciiay
Copy link
Copy Markdown
Member Author

ciiay commented Nov 21, 2022

@ciiay according to that comment we will disable it by default, but users should have the option to enable it - and we should document that enabling the dynamic plugin means there will be 2 environment tabs rendered in the console

This will require changes in the CRD to add a new field to enable/disable teh dynamic plugin, and the operator would need to be able to reconcile the plugin based on that setting in the CR

For now the SDK is not supporting to set Console Plugin default to Enable, so we have to only reconcile it once the static plugin gets removed on OCP 4.15.

Where else to document the 2 environment buttons rendered in the console besides GITOPS-2369 and GITOPS-2415?

@jaideepr97
Copy link
Copy Markdown
Contributor

jaideepr97 commented Nov 21, 2022

@ciiay according to that comment we will disable it by default, but users should have the option to enable it - and we should document that enabling the dynamic plugin means there will be 2 environment tabs rendered in the console

This will require changes in the CRD to add a new field to enable/disable teh dynamic plugin, and the operator would need to be able to reconcile the plugin based on that setting in the CR

For now the SDK is not supporting to set Console Plugin default to Enable, so we have to only reconcile it once the static plugin gets removed on OCP 4.15.

Where else to document the 2 environment buttons rendered in the console besides GITOPS-2369 and GITOPS-2415?

I'm not talking about enabling by default...I'm saying last I knew we were going to keep it disabled by default for < 4.15 but were still going to support enabling the dynamic plugin via the CR on OCP < 4.15 if the user explicitly wished to do so
I'm just wondering if that has changed

@ciiay
Copy link
Copy Markdown
Member Author

ciiay commented Nov 21, 2022

@ciiay according to that comment we will disable it by default, but users should have the option to enable it - and we should document that enabling the dynamic plugin means there will be 2 environment tabs rendered in the console
This will require changes in the CRD to add a new field to enable/disable teh dynamic plugin, and the operator would need to be able to reconcile the plugin based on that setting in the CR

For now the SDK is not supporting to set Console Plugin default to Enable, so we have to only reconcile it once the static plugin gets removed on OCP 4.15.
Where else to document the 2 environment buttons rendered in the console besides GITOPS-2369 and GITOPS-2415?

I'm not talking about enabling by default...I'm saying last I knew we were going to keep it disabled by default for < 4.15 but were still going to support enabling the dynamic plugin via the CR on OCP < 4.15 if the user explicitly wished to do so I'm just wondering if that has changed

There are two definition of Enable.

  1. Creating resources for dynamic plugin -> this has to happen only when static plugin gets removed which is planned on OCP 4.15
  2. Enable in Console Plugin section on gitops-operator installation page -> this will be available as long as dynamic plugin resources are created

We want to keep only one “Environments” button, so to avoid two buttons showing up, we only create dynamic plugins when static plugin is no longer there

addKnownTypesToScheme(s)

fakeClient := fake.NewFakeClient(newGitopsService())
fakeClient := fake.NewFakeClient(util.NewClusterVersion("4.15.1"), newGitopsService())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ciiay is this required here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@jaideepr97 Yes, it's needed, because I’m checking the OCP version in Reconcile function, if I don’t set it in the unit tests, it will through error and unit tests will fail. Or is there a better way to solve the problem?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

},
}
fakeClient := fake.NewFakeClient(gitopsService)
fakeClient := fake.NewFakeClient(util.NewClusterVersion("4.12.1"), gitopsService)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this required here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same as the previous one, if I don't set it here the unit test fails.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ciiay ciiay requested a review from jaideepr97 November 21, 2022 23:00
@iam-veeramalla
Copy link
Copy Markdown
Contributor

iam-veeramalla commented Nov 22, 2022

@ciiay @jaideepr97 @keithchong
Is there a way to use the new dynamic plugin on OCP 4.12 ? If yes, how ?

The below logic added in the PR seems to ignore the creation of plugin resources if OCP version < 4.15, Am I missing something ?

	v2, err := version.NewVersion("4.15.0")
	if v1.LessThan(v2) {
		return reconcile.Result{}, err
	} else {
		return r.reconcilePlugin(instance, request)
	}

@jaideepr97
Copy link
Copy Markdown
Contributor

jaideepr97 commented Nov 22, 2022

@ciiay @jaideepr97 @keithchong
Is there a way to use the new dynamic plugin on OCP 4.12 ? If yes, how ?

The below logic added in the PR seems to ignore the creation of plugin resources if OCP version < 4.15, Am I missing something ?

v2, err := version.NewVersion("4.15.0")
if v1.LessThan(v2) {
return reconcile.Result{}, err
} else {
return r.reconcilePlugin(instance, request)
}

@iam-veeramalla that is what i have asked above, it seems the decision is to only allow creation of dynamic plugin resources starting 4.15 (not even provide the option to enable it by the user)

so im actually not sure how anyone would be able to test this until we have a 4.15 cluster

cc @ciiay @keithchong

Comment thread controllers/gitopsservice_controller.go Outdated
}
v2, err := version.NewVersion("4.15.0")
if v1.LessThan(v2) {
return reconcile.Result{}, err
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ciiay I don't think we need to return an error here, we can just have

if !v1.LessThan(v2) {
     r.reconcilePlugin(instnace, request)
    }

@ciiay ciiay force-pushed the gitops-2415-add-reconcile-plugin-condition branch from 6f9831a to 9b0388e Compare November 22, 2022 16:03
@ciiay ciiay force-pushed the gitops-2415-add-reconcile-plugin-condition branch from 05d42b8 to f3a7d47 Compare November 22, 2022 16:19
@jaideepr97
Copy link
Copy Markdown
Contributor

@ciiay Please add sign-offs to all commits so the DCO check passes

@ciiay ciiay closed this Nov 22, 2022
@ciiay ciiay force-pushed the gitops-2415-add-reconcile-plugin-condition branch from 84f58b9 to 4e42591 Compare November 22, 2022 21:07
@ciiay ciiay reopened this Nov 22, 2022
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Nov 22, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jopit for approval by writing /assign @jopit in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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
Copy link
Copy Markdown

openshift-ci Bot commented Nov 22, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jopit for approval by writing /assign @jopit in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@ciiay ciiay force-pushed the gitops-2415-add-reconcile-plugin-condition branch from 84f58b9 to d1aee7d Compare November 22, 2022 21:13
@ciiay ciiay force-pushed the gitops-2415-add-reconcile-plugin-condition branch from e4de905 to 90efe71 Compare November 22, 2022 21:24
@ciiay ciiay force-pushed the gitops-2415-add-reconcile-plugin-condition branch from 2dd8a23 to aa55d06 Compare November 22, 2022 21:35
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Nov 22, 2022

@ciiay: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v4.8-e2e 0a35916 link true /test v4.8-e2e
ci/prow/v4.10-e2e 0a35916 link true /test v4.10-e2e

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

…perator into gitops-2415-add-reconcile-plugin-condition

Signed-off-by: Yi Cai <yicai@redhat.com>
@ciiay ciiay force-pushed the gitops-2415-add-reconcile-plugin-condition branch from 1076342 to cd79612 Compare November 22, 2022 21:40
@ciiay
Copy link
Copy Markdown
Member Author

ciiay commented Nov 22, 2022

New PR created #402. Closing this PR due to rebase and sign off issue.

@ciiay ciiay closed this Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants