Skip to content

Conversation

mgoerens
Copy link
Contributor

@mgoerens mgoerens commented Sep 9, 2025

No description provided.

This was referenced Sep 9, 2025
@mgoerens mgoerens force-pushed the refactor_check_locks branch from dd6896b to d76748b Compare September 9, 2025 15:51
This was referenced Sep 12, 2025
@komish
Copy link
Contributor

komish commented Sep 12, 2025

@komish Thanks for your comments. Please also note that there is a difference between the current implementation and this replacement: The script now returns a non-zero value when running into an issue, which makes the workflow to stop. I believe that change is fine, as the rest of the workflows are anyway not relevant in case this is not a PR with a single OWNERS file addition/modification.

As an aside, this also makes the merge_pr github_output redundant. I'll remove in another iteration on this workflow.

Great point.

I actually think the original intention here was to emit a message to the user when merge_pr was false. What I'm observing, however, is that the workflow just continues.

In other cases where merge_pr was being used, it's being used to inform... well, that a pr should be merged from that specific step's perspective. Combine that with merge_pr=true from other steps, and you effectively gate the inevitable outcome of PR merging.

In this case, community/redhat charts were never automerged anyway, so the only outcome we'd have is being able to provide user feedback via comments. At least for the community/redhat categories, I don't see logic to actually pass through that feedback coming from this workflow. To that end, we can fail the workflow for now. That seems like an overall improvement.

Copy link
Contributor

@komish komish left a comment

Choose a reason for hiding this comment

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

/lgtm

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.

2 participants