feat: create tiertemplaterevision for each tiertemplate #1103
Conversation
mfrancisc
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan and
ranakan19
as code owners
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mfrancisc 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 |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1103 +/- ##
==========================================
- Coverage 84.24% 79.55% -4.69%
==========================================
Files 55 78 +23
Lines 6327 7920 +1593
==========================================
+ Hits 5330 6301 +971
- Misses 826 1434 +608
- Partials 171 185 +14
|
| ns, err := configuration.GetWatchNamespace() | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
You can take the host namespace from the NSTemplateTier, right? this will also simplify the unit testing
| ns, err := configuration.GetWatchNamespace() | ||
| if err != nil { | ||
| return false, err | ||
| } |
| } | ||
|
|
||
| // check if there is TTR associated with this TierTemplate | ||
| ttrCreated, err = r.ensureTTRforTemplate(ctx, nsTmplTier, tierTemplate) |
There was a problem hiding this comment.
the final value of ttrCreated will always match the last returned value of the last TierTemplate, not a combination of all of them.
You need to use a logical OR with the already existing value in the var
| if created, err := r.ensureRevision(ctx, tier); err != nil { | ||
| return reconcile.Result{}, errs.Wrap(err, "unable to create new TierTemplateRevision after NSTemplateTier changed") | ||
| } else if created { | ||
| logger.Info("Requeue after creating a new TTR") |
There was a problem hiding this comment.
wouldn't make more sense to watch the TTR CRs? After all, the controller is creating them so it should really watch them.
| Labels: labels, | ||
| }, | ||
| Spec: toolchainv1alpha1.TierTemplateRevisionSpec{ | ||
| TemplateObjects: tierTmpl.Spec.TemplateObjects, |
There was a problem hiding this comment.
this should be formatted with parameters from the NSTemplateTier, right?
| return false, nil | ||
| } | ||
|
|
||
| func (r *Reconciler) ensureTTRforTemplate(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, tierTemplate toolchainv1alpha1.TierTemplate) (bool, error) { |
There was a problem hiding this comment.
correct me if I'm wrong, it looks like that the logic doesn't take into account removal of an existing TierTemplate ref from the NSTemplateTier.
|
|
||
| func (r *Reconciler) createTTR(ctx context.Context, tmplTier *toolchainv1alpha1.TierTemplate) (*toolchainv1alpha1.TierTemplateRevision, error) { | ||
| ttr := NewTTR(tmplTier) | ||
| if err := controllerutil.SetControllerReference(tmplTier, ttr, r.Scheme); err != nil { |
There was a problem hiding this comment.
the TTR CR is created for a combination of the TierTemplate content and NSTemplateTier parameters. The TTR CR becomes irrelevant as soon as either of the associated CRs is deleted. This means that we will need to have a special cleanup logic anyway.
|
|
||
| // check if the `status.revisions` field is up-to-date and create a TTR for each TierTemplate | ||
| if created, err := r.ensureRevision(ctx, tier); err != nil { | ||
| return reconcile.Result{}, errs.Wrap(err, "unable to create new TierTemplateRevision after NSTemplateTier changed") |
There was a problem hiding this comment.
Let's stop using the deprecated errrs.Wrap function in new code
|
|
||
| // NewTTR creates a TierTemplateRevision CR for a given TierTemplate object. | ||
| func NewTTR(tierTmpl *toolchainv1alpha1.TierTemplate) *toolchainv1alpha1.TierTemplateRevision { | ||
| tierName := tierTmpl.Spec.TierName |
There was a problem hiding this comment.
I think that the TierTemplate.Spec.TierName should go away from the TierTemplate CRs - they won't be tied to a specific tier, one TierTemplate could be reused in multiple NSTemplateTiers
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
passing t *testing.T as a parameter of the function and failing via
require.NoError(t, err)would simplify the code from the "caller point of view"
This PR introduces creation of TierTemplateRevision CRs for each TierTemplate, as soon as this is merged we should not see any TTR, we will start seeing those in host namespace only when we populate TierTemplate.Spec.TemplateObjects field.
With the current logic the TTRs are created only when there is a new TierTemplate ( thus one per TierTemplate ) with the TierTemplate.Spec.TemplateObjects field populated. The TTRs are not used yet, thus they will not guide the creation/update/deletion of user namespace content.
In follow up PRs we will introduce comparison between TierTemplate and TierTemplateRevision and usage of TierTemplateRevisions for driving space provisioning.
Jira: https://issues.redhat.com/browse/KUBESAW-146
Paired with: codeready-toolchain/toolchain-e2e#1061