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

use SSA to apply the template objects #634

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Mar 6, 2025

This is a minimal change in the member operator to start using the SSA to persist the changes to the templates.

I didn't do this change in toolchain-common to limit the "impact" this change would have at runtime across the ecosystem (i.e. not impact host-operator or ksctl).

The future steps for this would be to drastically simplify the template apply logic in toolchain-common.

As such, this is still more of an experiment than a definitive solution. At best, it is the first step in the larger effort to simplify the deployment of NSTemplateSets.

Copy link

openshift-ci bot commented Mar 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: metlos

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 Mar 6, 2025
@metlos metlos force-pushed the push-lopkqvnpmmow branch from fcaa95d to aee1f7f Compare March 6, 2025 12:31
@metlos metlos force-pushed the push-lopkqvnpmmow branch from aee1f7f to dc4e7ed Compare March 6, 2025 12:42
Copy link

sonarqubecloud bot commented Mar 6, 2025

@metlos metlos marked this pull request as draft March 6, 2025 13:49
@metlos
Copy link
Contributor Author

metlos commented Mar 6, 2025

/test e2e

Copy link

codecov bot commented Mar 6, 2025

Codecov Report

Attention: Patch coverage is 81.57895% with 7 lines in your changes missing coverage. Please review.

Project coverage is 81.73%. Comparing base (fc36345) to head (dc4e7ed).

Files with missing lines Patch % Lines
controllers/nstemplateset/client.go 81.57% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #634      +/-   ##
==========================================
+ Coverage   81.71%   81.73%   +0.02%     
==========================================
  Files          29       29              
  Lines        3330     3334       +4     
==========================================
+ Hits         2721     2725       +4     
  Misses        459      459              
  Partials      150      150              
Files with missing lines Coverage Δ
...trollers/nstemplateset/nstemplateset_controller.go 70.58% <ø> (ø)
controllers/nstemplateset/client.go 83.01% <81.57%> (+1.38%) ⬆️
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MatousJobanek
Copy link
Contributor

Thanks a lot for the PR 👍

I didn't do this change in toolchain-common to limit the "impact" this change would have at runtime across the ecosystem (i.e. not impact host-operator or ksctl).

I would simply create a new function "next" to the existing one (in a separate file/package) and gradually propagate the change to all places in our code.
In this way, it would be better to isolate & review the actual "apply" code in toolchain-common repo, and also isolate the changes in separate PRs while propagating the new logic to the respective places in our operators.

In other words, can you please move the "apply" logic to toolchain-common?

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.

2 participants