Skip to content

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Dec 9, 2024

Rationale for this change

The current implementation works for GH-XXX PRs but doesn't work for MINOR PRs.

What changes are included in this PR?

Add a missing issue.kind check.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@pitrou pitrou marked this pull request as ready for review December 9, 2024 13:45
@pitrou
Copy link
Member Author

pitrou commented Dec 9, 2024

Not sure how to test this before it reaches git main @raulcd

@github-actions
Copy link

github-actions bot commented Dec 9, 2024

⚠️ GitHub issue #44974 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added the awaiting review Awaiting review label Dec 9, 2024
Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

I've tested on my fork.
In order to test it I merge the change on my fork on main and create a PR against my fork, see this PR as an example:
raulcd#86
I had to also apply this change:
raulcd@c297025
otherwise it fails on the Check issue step as seen on this action failure:
https://github.com/raulcd/arrow/actions/runs/12237252551/job/34132570615
with my change applied it is successful as seen here:
https://github.com/raulcd/arrow/actions/runs/12237520289/job/34133461978?pr=86
I've also created a PR with an associated issue with GH- prefix, which is successful and performs the required steps. See here:
https://github.com/raulcd/arrow/actions/runs/12237616405/job/34133767858?pr=88
and the PR: raulcd#88

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Dec 9, 2024
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou kou merged commit f1b293d into apache:main Dec 9, 2024
10 checks passed
@kou kou removed the awaiting changes Awaiting changes label Dec 9, 2024
@github-actions github-actions bot added the awaiting merge Awaiting merge label Dec 9, 2024
@pitrou pitrou deleted the gh44974-issue-kind branch December 9, 2024 21:58
@raulcd
Copy link
Member

raulcd commented Dec 9, 2024

This is not going to fix the issue, it still requires this commit to be included:
raulcd@c297025

@kou
Copy link
Member

kou commented Dec 10, 2024

Oh, sorry. I checked only changes. I should have also checked affected file names.

kou pushed a commit that referenced this pull request Dec 10, 2024
### Rationale for this change

This is a follow up from: #44975 (comment)

### What changes are included in this PR?

Fix the issue_check workflow to not assume all PRs are going to have an associated issue. Fix the worflow for minor issues.

### Are these changes tested?

I've tested on my fork.

### Are there any user-facing changes?
No

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit f1b293d.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

QuietCraftsmanship pushed a commit to QuietCraftsmanship/arrow that referenced this pull request Jul 7, 2025
### Rationale for this change

This is a follow up from: apache/arrow#44975 (comment)

### What changes are included in this PR?

Fix the issue_check workflow to not assume all PRs are going to have an associated issue. Fix the worflow for minor issues.

### Are these changes tested?

I've tested on my fork.

### Are there any user-facing changes?
No

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting merge Awaiting merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants