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(cloudevents): fix not add the not runned steps in notificatioins #101

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

wuhuizuo
Copy link
Contributor

Signed-off-by: wuhuizuo [email protected]

Copy link

ti-chi-bot bot commented Mar 22, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the pull request title and description, it seems that the changes are related to CloudEvents and Tekton. The changes seem to fix an issue where not runned steps were not added in notifications.

In the actual code diff, the changes include adding a conditional statement that checks if the step is completed and only adds it to the notifications if it is completed. This seems like a reasonable fix.

However, there are a few potential problems that could arise from this change. Firstly, if there are cases where notifications need to be sent for steps that are not completed, then this fix would break that functionality. Additionally, there could be other areas of the code that rely on the previous behavior of adding all steps, regardless of completion status, to notifications.

To fix these issues, it may be worth discussing with the developer who submitted the pull request to understand the reasoning behind the change and if there are any other areas of the code that could be impacted. If necessary, tests should be added to ensure that the code continues to function as expected. Additionally, it may be worthwhile to update the pull request description to provide more context on the changes and their impact.

Copy link
Contributor Author

@wuhuizuo wuhuizuo left a comment

Choose a reason for hiding this comment

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

/approve

@ti-chi-bot ti-chi-bot bot added the size/XS label Mar 22, 2024
Copy link

ti-chi-bot bot commented Mar 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Mar 22, 2024
@ti-chi-bot ti-chi-bot bot merged commit edad397 into main Mar 22, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/just-stop-add-not-run-steps branch March 22, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant