You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "raulcd (via GitHub)" <gi...@apache.org> on 2023/06/22 13:54:47 UTC
[GitHub] [arrow] raulcd opened a new pull request, #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
raulcd opened a new pull request, #36244:
URL: https://github.com/apache/arrow/pull/36244
### Rationale for this change
Those labels are unnecessary once they are merged. There was a conversation on Zulip about removing them in the past.
### What changes are included in this PR?
Once we merge a PR we remove labels that starts with the PR workflow prefix `awaiting`.
### Are these changes tested?
I have tested the code against an old testing PR I have here: https://github.com/apache/arrow/pull/35323
The label was removed successfully.
### Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #36244:
URL: https://github.com/apache/arrow/pull/36244#discussion_r1238666597
##########
dev/merge_arrow_pr.py:
##########
@@ -418,11 +418,23 @@ def merge_pr(self, number, commit_title, commit_message):
}
response = requests.put(url, headers=self.headers, json=payload)
result = response.json()
- if response.status_code != 200 and 'merged' not in result:
+ if response.status_code == 200 and 'merged' in result:
Review Comment:
btw based on the API if the result is 200 the merge was successful and `merged` will always be true.
https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#merge-a-pull-request
I am sure we can mark this as `False` if those are not successful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #36244:
URL: https://github.com/apache/arrow/pull/36244#discussion_r1238596542
##########
dev/merge_arrow_pr.py:
##########
@@ -421,8 +421,19 @@ def merge_pr(self, number, commit_title, commit_message):
if response.status_code != 200 and 'merged' not in result:
result['merged'] = False
result['message'] += f': {url}'
+ else:
+ self.clear_pr_state_labels(number)
return result
+ def clear_pr_state_labels(self, number):
+ url = f'{self.github_api}/issues/{number}/labels'
+ response = requests.get(url, headers=self.headers)
+ labels = response.json()
+ for label in labels:
+ if label['name'].startswith('awaiting'):
+ label_url = f"{url}/{label['name']}"
+ response = requests.delete(label_url, headers=self.headers)
Review Comment:
```suggestion
requests.delete(label_url, headers=self.headers)
```
##########
dev/merge_arrow_pr.py:
##########
@@ -421,8 +421,19 @@ def merge_pr(self, number, commit_title, commit_message):
if response.status_code != 200 and 'merged' not in result:
result['merged'] = False
result['message'] += f': {url}'
+ else:
+ self.clear_pr_state_labels(number)
Review Comment:
How about invert the condition for easy to read?
```suggestion
if response.status_code == 200 or 'merged' in result:
self.clear_pr_state_labels(number)
else:
result['merged'] = False
result['message'] += f': {url}'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #36244:
URL: https://github.com/apache/arrow/pull/36244#issuecomment-1602686882
@pitrou I think you asked for this in the past. I can do a cleanup of labels once this is merged so we remove them from closed PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou merged pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #36244:
URL: https://github.com/apache/arrow/pull/36244
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #36244:
URL: https://github.com/apache/arrow/pull/36244#discussion_r1238628706
##########
dev/merge_arrow_pr.py:
##########
@@ -421,8 +421,19 @@ def merge_pr(self, number, commit_title, commit_message):
if response.status_code != 200 and 'merged' not in result:
result['merged'] = False
result['message'] += f': {url}'
+ else:
+ self.clear_pr_state_labels(number)
return result
+ def clear_pr_state_labels(self, number):
+ url = f'{self.github_api}/issues/{number}/labels'
+ response = requests.get(url, headers=self.headers)
+ labels = response.json()
+ for label in labels:
+ if label['name'].startswith('awaiting'):
Review Comment:
yes, all workflow labels start with the `awaiting` prefix. I will add a comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #36244:
URL: https://github.com/apache/arrow/pull/36244#discussion_r1238632029
##########
dev/merge_arrow_pr.py:
##########
@@ -421,8 +421,19 @@ def merge_pr(self, number, commit_title, commit_message):
if response.status_code != 200 and 'merged' not in result:
result['merged'] = False
result['message'] += f': {url}'
+ else:
+ self.clear_pr_state_labels(number)
Review Comment:
I think so, have update to:
```
if response.status_code == 200 and 'merged' in result:
self.clear_pr_state_labels(number)
else:
result['merged'] = False
result['message'] += f': {url}'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36244:
URL: https://github.com/apache/arrow/pull/36244#discussion_r1238614602
##########
dev/merge_arrow_pr.py:
##########
@@ -421,8 +421,19 @@ def merge_pr(self, number, commit_title, commit_message):
if response.status_code != 200 and 'merged' not in result:
result['merged'] = False
result['message'] += f': {url}'
+ else:
+ self.clear_pr_state_labels(number)
return result
+ def clear_pr_state_labels(self, number):
+ url = f'{self.github_api}/issues/{number}/labels'
+ response = requests.get(url, headers=self.headers)
+ labels = response.json()
+ for label in labels:
+ if label['name'].startswith('awaiting'):
Review Comment:
Do all workflow labels start with "awaiting"? In any case, add a comment explaining this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36244:
URL: https://github.com/apache/arrow/pull/36244#discussion_r1238613590
##########
dev/merge_arrow_pr.py:
##########
@@ -421,8 +421,19 @@ def merge_pr(self, number, commit_title, commit_message):
if response.status_code != 200 and 'merged' not in result:
result['merged'] = False
result['message'] += f': {url}'
+ else:
+ self.clear_pr_state_labels(number)
Review Comment:
Is there a logic error here? Should we clear the PR labels only if "merged" appears in the result?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36244:
URL: https://github.com/apache/arrow/pull/36244#issuecomment-1602684674
:warning: GitHub issue #36243 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36244: GH-36243: [Dev] Remove PR workflow label as part of merge
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36244:
URL: https://github.com/apache/arrow/pull/36244#issuecomment-1610668458
Conbench analyzed the 6 benchmark runs on commit `5c2c7acb`.
There were 8 benchmark results indicating a performance regression:
- Commit Run on `ursa-thinkcentre-m75q` at [2023-06-27 02:18:21Z](http://conbench.ursa.dev/compare/runs/6de8c1cf0ab045839562c49a24701a2f...c6f8571d52e741ee991edd34ec455126/)
- [params=<STATIC_VECTOR(std::shared_ptr<int>)>, source=cpp-micro, suite=arrow-small-vector-benchmark](http://conbench.ursa.dev/compare/benchmarks/0649a20424447d6f80006928234c6625...0649a472b88673898000fbb92d48799e)
- Commit Run on `arm64-t4g-linux-compute` at [2023-06-25 07:22:59Z](http://conbench.ursa.dev/compare/runs/c6c0325b2c764dc89323bf902b7a29a2...9d8de8fb470041b9bd11425b89790853/)
- [params=simple_expression/batch_size:10000/real_time, source=cpp-micro, suite=arrow-acero-project-benchmark](http://conbench.ursa.dev/compare/benchmarks/064965e66362724380000146a3f1ca2d...06497ebaca9177088000f327c43628d2)
- and 6 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/14611300835) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org