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.

Sign up for GitHub

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

Jump to bottom

OLS-342, OLS-347: creation and deletion of console plugin #30

Merged
merged 1 commit into from
Mar 16, 2024
Merged

OLS-342, OLS-347: creation and deletion of console plugin #30

merged 1 commit into from
Mar 16, 2024

Conversation

raptorsun
Copy link
Contributor

@raptorsun raptorsun commented Mar 14, 2024 •
edited
Loading

Description

This PR contain the creation and deletion of the console plugin.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up dependent library

Related Tickets & Documents

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

@openshift-ci openshift-ci bot requested review from bparees and xrajesh March 14, 2024 12:14
@raptorsun raptorsun changed the title UI plugin removal OLS-347: UI plugin removal Mar 14, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 14, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 14, 2024 •
edited by openshift-ci bot
Loading

@raptorsun: This pull request references OLS-347 which is a valid jira issue.

In response to this:

Description

This PR is on top of PR#26. Merge this will bring everything from PR#26 (OLS-342 Deploy ols console plugin using ols operator).
This PR removes the console plugin and removes the lightspeed console plugin from the activated plugin list.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up dependent library

Related Tickets & Documents

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

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 openshift-eng/jira-lifecycle-plugin repository.

@raptorsun raptorsun mentioned this pull request Mar 14, 2024
Closed
11 tasks
@raptorsun raptorsun changed the title OLS-347: UI plugin removal OLS-342, OLS-347: creation and deletion of console plugin Mar 14, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 14, 2024 •
edited by openshift-ci bot
Loading

@raptorsun: This pull request references OLS-347 which is a valid jira issue.

In response to this:

Description

This PR contain the creation and deletion of the console plugin.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up dependent library

Related Tickets & Documents

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

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 openshift-eng/jira-lifecycle-plugin repository.

vbelouso
vbelouso reviewed Mar 15, 2024
View reviewed changes
internal/controller/ols_console_reconciliator.go Outdated
if err != nil {
return fmt.Errorf("failed to create Console UI configmap: %w", err)
}
r.logger.Info("Console configmap created", "configmap", cm.Name)
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 we need to be consistent in logging.
In other resources we don't log resource creation, only the error during creation.

internal/controller/ols_console_reconciliator.go Outdated
}

if deploymentSpecEqual(&foundDeployment.Spec, &deployment.Spec) {
r.logger.Info("Console UI deployment unchanged", "deployment", deployment.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here
Throughout the code we use ".... reconciliation skipped"

internal/controller/ols_console_reconciliator.go Outdated Show resolved Hide resolved
internal/controller/ols_console_reconciliator.go Outdated Show resolved Hide resolved
internal/controller/ols_console_reconciliator.go Outdated
return nil
}

plugin.SetResourceVersion(foundPlugin.GetResourceVersion())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we doing this?
Do we want to indicate that the resource has changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the k8s client refuse to update the resource in absence of resrouce version.
we can also just replace the spec of foundPlugin with generated one and use the foundPlugin to update the resource.

internal/controller/ols_console_reconciliator_test.go
Name: ConsoleCRName,
},
Spec: openshiftv1.ConsoleSpec{
Plugins: []string{"monitoring-plugin"},
Copy link
Contributor

@vbelouso vbelouso Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

must be lightspeed-console-plugin for the entire PR

Copy link
Contributor Author

@raptorsun raptorsun Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This minics the Console resource in the openshift cluster, it has "monitoring-plugin" activated by default.
At the end of this test case, we check this list contains "lightspeed-console-plugin" (at line 88)

internal/controller/ols_console_ui_assets.go Outdated Show resolved Hide resolved
internal/controller/ols_console_ui_assets.go Outdated Show resolved Hide resolved
@raptorsun
Copy link
Contributor Author

Thank you for the review @vbelouso :)
I have pushed a new commit that removes unneccesary default value settings in asset definition, added a SetDefaults_Deployment function to fill up the gap before comparing generated deployment with the existing one.
Please have another look.

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Mar 15, 2024

@raptorsun: all tests passed!

Full PR test history. Your PR dashboard.

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.

@vbelouso
Copy link
Contributor

Thank you for the review @vbelouso :) I have pushed a new commit that removes unneccesary default value settings in asset definition, added a SetDefaults_Deployment function to fill up the gap before comparing generated deployment with the existing one. Please have another look.

I'm OK with that, but perhaps in the future we will need to explore additional options for comparing objects more natively.
Interesting thread kubernetes-sigs/kubebuilder#592

@vbelouso
Copy link
Contributor

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Mar 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vbelouso

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 Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit cba45c1 into openshift:main Mar 16, 2024
5 checks passed
@raptorsun raptorsun mentioned this pull request Mar 19, 2024
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbelouso vbelouso vbelouso left review comments

@bparees bparees Awaiting requested review from bparees

@xrajesh xrajesh Awaiting requested review from xrajesh

Assignees

@vbelouso vbelouso

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raptorsun @openshift-ci-robot @vbelouso