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

[CI][Go] Run benchmark steps only on the main branch #36218

Closed
kou opened this issue Jun 21, 2023 · 0 comments · Fixed by #36219 or #36229
Closed

[CI][Go] Run benchmark steps only on the main branch #36218

kou opened this issue Jun 21, 2023 · 0 comments · Fixed by #36219 or #36229

Comments

@kou
Copy link
Member

kou commented Jun 21, 2023

Describe the enhancement requested

Is there any reason we are raising an AssertionError here?

Yes, this code is not supposed to execute in GHA for a non-default branch:

"pr_number": None, # implying default branch

else:
    # Assume that the environment is not GitHub Actions CI. Error out if that
    # assumption seems to be wrong.
    assert os.getenv("GITHUB_ACTIONS") is None

    # This is probably a local dev environment, for testing. In this case, it
    # does usually not make sense to provide commit information (not a
    # controlled CI environment). Explicitly keep `github_commit_info=None` to
    # reflect that (to not send commit information).

In that sense I think the failure is expected and does / should not block merge. If the redness is a problem then we can also sys.exit(0) instead. Although that might result in mean false-positive green signal in the future.

Originally posted by @jgehrcke in #36195 (comment)


Ok, I think the problem is that the PR from dependabot is coming from a branch on the apache/arrow repository instead of a fork hence those are executed which are not if they are from a fork branch due to:
if: success() && github.event_name == 'push' && github.repository == 'apache/arrow'
Probably we should not error and just do a sys.exit(0) but now I understand why this is failing here.

Originally posted by @raulcd in #36195 (comment)

Component(s)

Continuous Integration, Go

assignUser pushed a commit that referenced this issue Jun 22, 2023
### Rationale for this change

Because our benchmark tool supports only the main branch.

### What changes are included in this PR?

Add branch name check to the existing `if:`s.

### Are these changes tested?

No.

### Are there any user-facing changes?

No.
* Closes: #36218 

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
@assignUser assignUser added this to the 13.0.0 milestone Jun 22, 2023
kou added a commit to kou/arrow that referenced this issue Jun 22, 2023
Sorry. This is a follow-up of apacheGH-36219. We need one more change for
this.
kou added a commit that referenced this issue Jun 22, 2023
### Rationale for this change

Sorry. This is a follow-up of GH-36219. We need one more change for this.

### What changes are included in this PR?

Add one more branch name check.

### Are these changes tested?

No.

### Are there any user-facing changes?

No.
* Closes: #36218

Authored-by: Sutou Kouhei <kou@clear-code.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