Repository navigation
chore: move Release structs from pkg/cluster to apis/common - #1580
Merged
openshift-merge-bot[bot] merged 1 commit intoJan 29, 2025
Merged
Conversation
lburgazzoli
force-pushed
the
api-cleanup
branch
from
January 29, 2025 09:11
77a0aee to
88b529c
Compare
ykaliuta
reviewed
Jan 29, 2025
Contributor
|
small typo in the commit message (aprt -> part) |
Structs that are part of the public APIs should be in the apis packages for consistency and to avoid cyclic import issues
lburgazzoli
force-pushed
the
api-cleanup
branch
from
January 29, 2025 10:17
88b529c to
f4e9e26
Compare
Member
Author
fixed |
ykaliuta
approved these changes
Jan 29, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1580 +/- ##
==========================================
+ Coverage 19.98% 20.00% +0.01%
==========================================
Files 159 158 -1
Lines 10780 10770 -10
==========================================
Hits 2154 2154
+ Misses 8396 8386 -10
Partials 230 230 ☔ View full report in Codecov by Sentry. |
lburgazzoli
changed the title
zdtsw
approved these changes
Jan 29, 2025
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ykaliuta, zdtsw The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Member
Author
|
/test opendatahub-operator-e2e |
Member
Author
|
/test all |
zdtsw
pushed a commit
to zdtsw-forking/opendatahub-operator
that referenced
this pull request
Feb 14, 2025
…hub-io#1580) Structs that are part of the public APIs should be in the apis packages for consistency and to avoid cyclic import issues (cherry picked from commit 1760dee)
5 tasks
openshift-merge-bot Bot
pushed a commit
that referenced
this pull request
Feb 14, 2025
…1666) * Improve gc service/action (#1656) * Improve gc service/action * Remove useless lock * Fix typos * Improve error handling * update: remove ondelete not used for cleanup steps (#1621) * update: remove ondelete not used for cleanup steps - we do not unpatch auth in smcp since we do not support unmanaged case --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> * actions: refactor to move template and manifest rendering to a helper (#1563) * actions: abstract caching functionality into cacher The caching code for all caching Actions is pretty much the same 1) hash the key 2) if the hash is the same as the stored one, return stored resource 3) otherwise render the resource 4) store the key and the resource 5) return the resource Abstract this logic into Cacher[T] generic type, where T is the type of the resource, which is supposed to be included as a composition in on the higher levels. The core engine is implemented as Render method which accepts actual renderer (function) and return boolean value in addition to the resources indicating if it performed rendering. It supposed to be used in resource rendering actions for accounting. The renderer should return (error, T). Put renderer as the last argument to make more convenient to call with a closure. Copy CachingKeyFn type. In future it can be extented to accept context for possible debug logging. Define contract, CachingKeyFn should not return empty hash. (!!!) The code originally is taken pretty much from the existing kustomize rendering Action. But taking into account the contract, refactor the code to avoid comparation of the result (and nil assignments) since with generics to work with both nilable (maps, arrays, etc) and non-nilable (ints, strings) types it requires some extra handling of Zero values. Zero values are still required for return values in error case, so implement Zero[T] generic function. With the assumption make "empty key" a error condition, it removes need of extra comparation. Move actual rendering and cache update into separate function, it makes main Render function more clear without extra indentation. Possible usage supposed to be ``` type Action struct { cacher.Cacher[resources.UnstructuredList] // action data } type ActionOpts func(*Action) func WithCache() ActionOpts { return func(a *Action) { a.Cacher.SetKeyFn(types.Hash) // or own } } func (s *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { //... res, acted, err := a.Cacher.Render(ctx, rr, s.render) if acted { /* may be do something */ } // use resources } func (s *Action) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { result := make(resources.UnstructuredList) // make some resources return result, nil } func NewAction(opts ...ActionOpts) actions.Fn { for _, opt := range opts { opt(&action) } return action.run } ``` with reconciler creation ``` func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { _, err := reconciler.ReconcilerFor( mgr, &componentApi.Type{}, ). // ... WithAction(foobar.NewAction( foobar.WithCache(), )) } ``` Pay attention to signature of render() and call to Cacher.Render() from run(). Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> * actions: abstract resource caching into resourcecacher Make an abstract wrapper on the raw cacher.Cacher[T] to work on actions whose Renderer produces resources (resources.UnstructuredList) to be recorded into ReconciliationRequest.Manifests and later deployed (deleted) to the cluster. Performs common tasks: - invalidate caching if devflags enabled; - account resources if they were generated (and skip cached ones); - add resources to the ReconciliationRequest. Created constructor which accepts mandatory name. Since it is used in prometheus, it's recommended to make it lower case. At this moment this is the only part which interested in the origin of resources (cached or not) and it's supposed to be used in actions whose final goal is to add resources to the request (which is done by this helper), so change signature of its Render() method (comparing to cacher.Cacher) to return only error. The code is basically copied from manifest/template rendering actions run() method. Usage (changed run() comparing to cacher.Cacher): ``` type Action struct { resourcecacher.ResourceCacher // action data } type ActionOpts func(*Action) func WithCache() ActionOpts { return func(a *Action) { a.ResourceCacher.SetKeyFn(types.Hash) // or own } } func (s *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { //... return a.ResourceCacher.Render(ctx, rr, s.render) } func (s *Action) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { result := make(resources.UnstructuredList) // make some resources return result, nil } func NewAction(opts ...ActionOpts) actions.Fn { action := Action{ ResourceCacher: resourcecacher.NewResourceCacher("mycoolaction"), } for _, opt := range opts { opt(&action) } return action.run } ``` Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> * actions: use resourcecacher for manifests rendering All the run() functionality now implemented by the ResourceCacher, so just call its Render() in run(). Amend WithCache() as required by ResourceCacher to set its CachingKeyFn and remove CachingKeyFn initialization from NewAction. Change render() signature to return resources.UnstructuredList RendererEngine is used as a field initializer now, so no need to export, rename to rendererEngine. Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> * actions: use resourcecacher for template rendering The same changes as to manifests rendering. Just keep `data` field initialization in NewAction, used in render(). Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> --------- Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> * chore: move Release strcuts from pkg/cluster to apis/common (#1580) Structs that are part of the public APIs should be in the apis packages for consistency and to avoid cyclic import issues (cherry picked from commit 1760dee) * feat(test): support for Succeed and MatchError in testf framework (#1628) (cherry picked from commit a774767) * chore(kserve): wrap error returned by FromUnstructured (#1646) (cherry picked from commit 9402f6a) --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> Co-authored-by: Luca Burgazzoli <lburgazzoli@users.noreply.github.com> Co-authored-by: Yauheni Kaliuta <ykaliuta@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).The developer has manually tested the changes and verified that the changes work