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

feat(RELEASE-1109): allow tenant and managed pipelines to run #549

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

johnbieren
Copy link
Collaborator

This commit modifies the release controller to execute both a tenant and managed pipelineRun in the same Release if both are defined.

Copy link

openshift-ci bot commented Aug 28, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Collaborator

@davidmogar davidmogar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just read it in diagonal and added some comments. We can discuss them offline (and maybe address them) before I do a proper review.

api/v1alpha1/release_types.go Outdated Show resolved Hide resolved
api/v1alpha1/release_types.go Outdated Show resolved Hide resolved
api/v1alpha1/release_types.go Outdated Show resolved Hide resolved
api/v1alpha1/release_types.go Outdated Show resolved Hide resolved
controllers/release/adapter.go Outdated Show resolved Hide resolved
controllers/release/adapter.go Outdated Show resolved Hide resolved
@johnbieren johnbieren force-pushed the release1109 branch 6 times, most recently from 5a38216 to cf0306f Compare September 3, 2024 19:44
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 87.93103% with 28 lines in your changes missing coverage. Please review.

Project coverage is 84.09%. Comparing base (9fab770) to head (9d2ba52).
Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
controllers/release/adapter.go 81.14% 12 Missing and 11 partials ⚠️
controllers/release/controller.go 0.00% 2 Missing ⚠️
loader/loader.go 75.00% 1 Missing and 1 partial ⚠️
loader/loader_mock.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #549      +/-   ##
==========================================
+ Coverage   83.75%   84.09%   +0.34%     
==========================================
  Files          26       26              
  Lines        1508     1635     +127     
==========================================
+ Hits         1263     1375     +112     
- Misses        173      182       +9     
- Partials       72       78       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metrics/release.go Outdated Show resolved Hide resolved
api/v1alpha1/release_conditions.go Show resolved Hide resolved
api/v1alpha1/release_conditions.go Show resolved Hide resolved
api/v1alpha1/release_types.go Show resolved Hide resolved
api/v1alpha1/release_types.go Outdated Show resolved Hide resolved
controllers/release/controller.go Outdated Show resolved Hide resolved
loader/loader.go Show resolved Hide resolved
metrics/release.go Outdated Show resolved Hide resolved
controllers/release/adapter.go Outdated Show resolved Hide resolved
controllers/release/adapter.go Show resolved Hide resolved
controllers/release/adapter.go Show resolved Hide resolved
Copy link
Collaborator

@davidmogar davidmogar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking ok to me once the open discussions are resolved.

@theflockers
Copy link
Collaborator

theflockers commented Sep 24, 2024

lgtm. Maybe link the new PR here (or the new ticket) of the work on the metrics will be done.

Copy link

openshift-ci bot commented Sep 24, 2024

New changes are detected. LGTM label has been removed.

This commit modifies the release controller to execute both a tenant and
managed pipelineRun in the same Release if both are defined.

Signed-off-by: Johnny Bieren <[email protected]>
@johnbieren
Copy link
Collaborator Author

lgtm. Maybe link the new PR here (or the new ticket) of the work on the metrics will be done.

Okay, I squashed the commits down then. Can you clarify? What new PR or ticket? What metrics work needs to be done? It sounds like this is something for a new ticket, but I will need some more details before filing it

@theflockers
Copy link
Collaborator

theflockers commented Sep 24, 2024

lgtm. Maybe link the new PR here (or the new ticket) of the work on the metrics will be done.

Okay, I squashed the commits down then. Can you clarify? What new PR or ticket? What metrics work needs to be done? It sounds like this is something for a new ticket, but I will need some more details before filing it

Sorry. I misunderstood what you said. I thought you wanted to do more changes besides the one I requested (which lgtm). So please ignore the comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants