feat(RELEASE-914): add collectors phase #586
Conversation
|
Skipping CI for Draft Pull Request. |
davidmogar
force-pushed
the
release914
branch
from
acb44e4
to
db69aa6
Compare
davidmogar
force-pushed
the
release914
branch
5 times, most recently
from
db2bd44
to
0dfe040
Compare
davidmogar
force-pushed
the
release914
branch
from
0dfe040
to
1444064
Compare
|
/retest |
| @@ -32,7 +32,11 @@ func isReleasePipelineRun(object client.Object) bool { | |||
|
|
|||
| labelValue, found := object.GetLabels()[metadata.PipelinesTypeLabel] | |||
|
|
|||
| return found && (labelValue == metadata.FinalPipelineType || labelValue == metadata.ManagedPipelineType || labelValue == metadata.TenantPipelineType) | |||
| return found && (labelValue == metadata.TenantCollectorsPipelineType || | |||
There was a problem hiding this comment.
Should probably have a test for this. It was missed with final pipeline but the rest have it. The test is very short
| @@ -254,7 +254,8 @@ func (l *loader) GetRoleBindingFromReleaseStatus(ctx context.Context, cli client | |||
| // GetReleasePipelineRun returns the Release PipelineRun of the specified type referenced by the given Release | |||
| // or nil if it's not found. In the case the List operation fails, an error will be returned. | |||
| func (l *loader) GetReleasePipelineRun(ctx context.Context, cli client.Client, release *v1alpha1.Release, pipelineType string) (*tektonv1.PipelineRun, error) { | |||
| if pipelineType != metadata.ManagedPipelineType && pipelineType != metadata.TenantPipelineType && pipelineType != metadata.FinalPipelineType { | |||
| if pipelineType != metadata.ManagedCollectorsPipelineType && pipelineType != metadata.ManagedPipelineType && | |||
There was a problem hiding this comment.
Should have test update
api/v1alpha1/release_types.go
Outdated
| conditions.SetCondition(&r.Status.Conditions, managedCollectorsProcessedConditionType, metav1.ConditionFalse, ProgressingReason) | ||
|
|
||
| go metrics.RegisterNewReleasePipelineProcessing( | ||
| r.Status.CollectorsProcessing.ManagedCollectorsProcessing.StartTime, |
There was a problem hiding this comment.
I wonder if we have a bug (it isn't just this function). It looks to me like the start time should be the first argument and the processing start the second
There was a problem hiding this comment.
I think we had multiple bugs in the metrics code which I fixed. This one I think you are right as well and I modified.
| @@ -455,12 +630,14 @@ func (r *Release) MarkReleased() { | |||
| go metrics.RegisterCompletedRelease( | |||
There was a problem hiding this comment.
arguments here don't appear in the right order. https://github.com/konflux-ci/release-service/blob/1444064e175d41cb856246f6e084e82a11fe377f/metrics/release.go has validationReason last for example, whereas here it is target
| @@ -491,12 +668,14 @@ func (r *Release) MarkReleaseFailed(message string) { | |||
| go metrics.RegisterCompletedRelease( | |||
controllers/release/adapter.go
Outdated
| @@ -717,6 +1027,25 @@ func (a *adapter) getEmptyReleaseServiceConfig(namespace string) *v1alpha1.Relea | |||
| return releaseServiceConfig | |||
| } | |||
|
|
|||
| // registerManagedCollectorsProcessingData adds all the Release Tenant Collectors processing information to its Status | |||
|
|
||
| return a.client.Status().Patch(a.ctx, a.release, patch) | ||
| } | ||
|
|
||
| // registerTenantProcessingStatus updates the status of the Release being processed by monitoring the status of the |
There was a problem hiding this comment.
I wonder why we set a latter stage as skipped in just this one
There was a problem hiding this comment.
Bit lost with this comment. Could you clarify?
controllers/release/adapter_test.go
Outdated
| Expect(adapter.release.IsManagedCollectorsPipelineProcessing()).To(BeFalse()) | ||
| }) | ||
|
|
||
| It("should continue and mark tenant collectors processing as skipped if the ReleasePlanAdmission have no collectors defined", func() { |
controllers/release/adapter_test.go
Outdated
| Expect(adapter.release.IsTenantCollectorsPipelineProcessing()).To(BeFalse()) | ||
| }) | ||
|
|
||
| It("should continue and mark tenant collectors processing as skipped if the ReleasePlan have no collectors defined", func() { |
| @@ -1465,37 +1760,206 @@ var _ = Describe("Release adapter", Ordered, func() { | |||
| BeforeEach(func() { | |||
There was a problem hiding this comment.
Couldn't this be AfterAll and BeforeAll instead of creating and deleting the pipelinerun between each run? Since no change is made before the creation
There was a problem hiding this comment.
Maybe but as we use AfterEach and BeforeEach in every single test to not condition tests within a when block, I kept it like that.
This change enables the execution of the collectors pipeline. Signed-off-by: David Moreno García <[email protected]>
davidmogar
force-pushed
the
release914
branch
from
1444064
to
90d28d0
Compare
|
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. |
This change enables the execution of the collectors pipeline.
Signed-off-by: David Moreno García [email protected]