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 |
| @@ -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.
davidmogar
force-pushed
the
release914
branch
from
1444064 to
90d28d0
Compare
davidmogar
force-pushed
the
release914
branch
from
90d28d0 to
ad6af30
Compare
| // +optional | ||
| Collectors []Collector `json:"collectors,omitempty"` | ||
| Collectors *Collectors `json:"collectors,omitempty"` |
There was a problem hiding this comment.
this requires the RP to look like this to be able to be applied:
---
apiVersion: appstudio.redhat.com/v1alpha1
kind: ReleasePlan
metadata:
labels:
release.appstudio.openshift.io/auto-release: 'true'
release.appstudio.openshift.io/standing-attribution: 'true'
name: collect-rp
spec:
application: rh-advisories
collectors:
items:
- name: scott-test
params:
- name: scott
value: tom
timeout: 60
type: scott-type
I don't think we want items there do we?
There was a problem hiding this comment.
We want to add a service account that is used by all the collectors so we had to add both keys, items and serviceAccountName.
There was a problem hiding this comment.
I think having this items key is really ugly and not intuitive as none of the lists we have in any other CRDs has you list items
| var revision string | ||
| var err error | ||
|
|
||
| if releasePlanAdmission != nil { |
There was a problem hiding this comment.
if you do not have a RPA, then revision is empty and the empty param value is passed....which is incorrect.
There was a problem hiding this comment.
True. Addressed.
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
ad6af30 to
334a430
Compare
| @@ -233,7 +371,8 @@ func (a *adapter) EnsureTenantPipelineIsProcessed() (controller.OperationResult, | |||
| // EnsureManagedPipelineIsProcessed is an operation that will ensure that a managed Release PipelineRun associated to the Release | |||
| // being processed and a RoleBinding to grant its serviceAccount permissions exist. Otherwise, it will create them. | |||
| func (a *adapter) EnsureManagedPipelineIsProcessed() (controller.OperationResult, error) { | |||
| if a.release.HasManagedPipelineProcessingFinished() || !a.release.HasTenantPipelineProcessingFinished() { | |||
| if a.release.HasManagedPipelineProcessingFinished() || !a.release.HasTenantPipelineProcessingFinished() || | |||
| !a.release.IsTenantPipelineProcessed() { | |||
There was a problem hiding this comment.
Is this needed though?
If a.release.HasTenantPipelineProcessingFinished() is not true, that means hasPhaseFinished(tenantProcessedConditionType) is not true. That means the condition is nil or the condition is false and the reason isn't progressing
If a.release.IsTenantPipelineProcessed() is not true, that means isStatusConditionTrue(r.Status.Conditions, tenantProcessedConditionType) is not true. So, the condition is false or unknown. I am pretty sure we only set it to unknown with reason progressing, so this seems like the same check but maybe I am missing something
| // +optional | ||
| Collectors []Collector `json:"collectors,omitempty"` | ||
| Collectors *Collectors `json:"collectors,omitempty"` |
There was a problem hiding this comment.
I think having this items key is really ugly and not intuitive as none of the lists we have in any other CRDs has you list items
This change enables the execution of the collectors pipeline.
Signed-off-by: David Moreno García [email protected]