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] add an allgood job for easy [required] builds #3351

Merged
merged 3 commits into from
Sep 6, 2020

Conversation

.github/workflows/main.yml Outdated Show resolved Hide resolved
@graingert
Copy link
Contributor Author

@jameslamb you should be able to add "required" to GitHub Actions / allgood (pull_request) Successful in 1s

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@graingert Thanks a lot! Indeed, this looks like something we were looking for!
Now we need just to mark this new job as required, right?

image

.github/workflows/main.yml Outdated Show resolved Hide resolved
@StrikerRUS StrikerRUS changed the title add an allgood job for easy [required] builds [ci] add an allgood job for easy [required] builds Sep 4, 2020
@jameslamb
Copy link
Collaborator

@StrikerRUS yeah, I think this is right: #3351 (review)

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@graingert Thanks a lot for the workaround!

And thanks for making wordings easier to understand, LGTM!

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

works for me, thanks!

@StrikerRUS @guolinke I'll switch the set of required checks after merging

@jameslamb jameslamb merged commit 636e4ee into microsoft:master Sep 6, 2020
@graingert graingert deleted the patch-1 branch September 6, 2020 01:33
@jameslamb
Copy link
Collaborator

oh nevermind, it looks like I don't have access to change the required checks.

@guolinke sorry to bother you, could you change the set of required checks for GitHub Actions? Now we only need to require the all-successful task and all others can be disabled. After that, we'll be able to add and remove jobs without needing you too change the settings again.

@guolinke
Copy link
Collaborator

guolinke commented Sep 6, 2020

@jameslamb no problem

@graingert
Copy link
Contributor Author

graingert commented Sep 6, 2020 via email

@jameslamb
Copy link
Collaborator

thanks @graingert . We change these settings rarely enough that I think it's better to have them done manually than to have another integration to maintain.

@@ -160,3 +160,10 @@ jobs:
$env:TASK = "${{ matrix.task }}"
conda init powershell
& "$env:GITHUB_WORKSPACE/.ci/test_windows.ps1"
all-successful:
Copy link

Choose a reason for hiding this comment

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

@graingert FYI this check may be problematic with failures and branch protection. Here's a more comprehensive solution: https://github.com/re-actors/alls-green.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice one!

Choose a reason for hiding this comment

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

Could you be more specific? How might this cause problems with branch protection?

Choose a reason for hiding this comment

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

@danepowell it's explained at https://github.com/marketplace/actions/alls-green#why but TL;DR — when some job dependencies fail, this job may end up being skipped and branch protection treats skipped jobs as success.

Choose a reason for hiding this comment

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

I've also filed #5501.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants