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

Refactor TemplateEngine and ClusterInstance packages #116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor TemplateEngine and ClusterInstance packages #116

wants to merge 1 commit into from

Conversation

sakhoury
Copy link

@sakhoury sakhoury commented Oct 8, 2024 •
edited
Loading

Summary

This PR refactors the TemplateEngine used to render install templates. The following changes are made:

  • Refactor TemplateEngine into an interface type defined under internal/templates/engine.
  • Define a concrete ClusterInstance TemplateEngine type (CITemplateEngine) that implements the interface and removes the state-based logger field from the former TemplateEngine struct.
  • Reorganized rendered_object.go source file (along with its unit tests) to internal/templates/engine/.
  • Refactor the clusterinstance package by separating the ClusterData into its own source file.
  • Refactor the clusterinstance package by moving commonly used functions and constants to internal/controller/common package.
  • Renamed the imagebasedinstall package to imagebasedinstaller.
  • Added Red Hat copyright headers to internal/controller/conditions and internal/controller/retry source files.

Note that no new functionality is added to this PR.

Testing

An end-to-end manual test was performed to verify the SiteConfig Operator functionality.

@sakhoury sakhoury requested a review from carbonin October 8, 2024 17:17
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sakhoury

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 Oct 8, 2024
@sakhoury sakhoury mentioned this pull request Oct 8, 2024
Merged
@sakhoury
Copy link
Author

sakhoury commented Oct 8, 2024

/override ci/prow/sonar-pre-submit

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 8, 2024

@sakhoury: Overrode contexts on behalf of sakhoury: ci/prow/sonar-pre-submit

In response to this:

/override ci/prow/sonar-pre-submit

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-sigs/prow repository.

carbonin
carbonin reviewed Oct 8, 2024
View reviewed changes
internal/controller/clusterinstance/template_engine.go

log := te.log.Named("ProcessTemplates")
// Type assertion to handle specific object types
clusterInstance, ok := object.(v1alpha1.ClusterInstance)
Copy link

Choose a reason for hiding this comment

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

Can this ever be something else? Why not pass the concrete type rather than interface{}?

Copy link
Author

Choose a reason for hiding this comment

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

The TemplateEngine interface is future-proof designed such that it can accept the processing of an arbitrary CR.

internal/templates/engine/rendered_object.go Outdated Show resolved Hide resolved
internal/templates/engine/template_engine_interface.go
)

// TemplateEngine defines an interface for processing templates for an abritary object
type TemplateEngine interface {
Copy link

Choose a reason for hiding this comment

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

I'm curious about your reasons for this change now, are we expecting other kinds of objects to be used to process templates (other than ClusterInstance)?

Copy link
Author

Choose a reason for hiding this comment

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

The SiteConfig Operator can potentially be used to consume and render new CR types other than the ClusterInstance (in the future). Hence, this refactoring, not only logically organizes the existing code for maintainability and readability, it also supports such avenues in the future.

@carbonin
Copy link

carbonin commented Oct 8, 2024 •
edited
Loading

I think also generally the bug fix for the logging state in the template engine (which would be a problem if we run multiple reconciles at once) should be separate from the rest of this refactoring. Especially if we're trying to get the logging change back into 2.12.

@sakhoury
Copy link
Author

sakhoury commented Oct 9, 2024 •
edited
Loading

I think also generally the bug fix for the logging state in the template engine (which would be a problem if we run multiple reconciles at once) should be separate from the rest of this refactoring. Especially if we're trying to get the logging change back into 2.12.

Fair enough! I'll create a separate PR with a fix to the potential template engine logging issue when we support multiple concurrent reconciles.

Update: here is the PR #119.

@sakhoury
Refactor TemplateEngine and ClusterInstance packages
38d004a 
This commit refactors the TemplateEngine used to render install templates. The following changes are made:
- Refactor TemplateEngine into an interface type defined under internal/templates/engine.
- Define a concrete ClusterInstance TemplateEngine type that implements the interface.
- Refactor the clusterinstance package by separating the ClusterData into its own source file.
- Refactor the clusterinstance package by moving commonly used functions and constants to internal/controller/common package.
- Reorganized rendered_object.go source file (along with its unit tests) to internal/templates/engine/.
- Renamed the imagebasedinstall package to imagebasedinstaller.
- Added Red Hat copyright headers to internal/controller/conditions and internal/controller/retry source files.

Signed-off-by: Sharat Akhoury <[email protected]>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 70%)

See analysis details on SonarCloud

@sakhoury
Copy link
Author

sakhoury commented Oct 9, 2024

/override ci/prow/sonar-pre-submit

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 9, 2024

@sakhoury: Overrode contexts on behalf of sakhoury: ci/prow/sonar-pre-submit

In response to this:

/override ci/prow/sonar-pre-submit

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-sigs/prow repository.

@openshift-merge-robot
Copy link

PR needs rebase.

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbonin carbonin carbonin left review comments

@eranco74 eranco74 Awaiting requested review from eranco74

@imiller0 imiller0 Awaiting requested review from imiller0

Assignees
No one assigned
Labels
approved dco-signoff: yes needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sakhoury @carbonin @openshift-merge-robot