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

Improve feedback from CI runs with all secrets #17927

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

nineinchnick
Copy link
Member

Description

When starting the CI run with all secrets, add a comment to the original PR with a link to that run, instead of only adding a comment after such run would fail. Update the comment with the status once it finishes.

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

When starting the CI run with all secrets, add a comment to the original
PR with a link to that run, instead of only adding a comment after such
run would fail. Update the comment with the status once it finishes.
@cla-bot cla-bot bot added the cla-signed label Jun 16, 2023
@ebyhr
Copy link
Member

ebyhr commented Jun 16, 2023

/test-with-secrets sha=1a56bc8816812b48e715d655addb2d930cf7eefa

Note: @nineinchnick told me we can't this change without merging PR.

.github/actions/update-check/action.yml Show resolved Hide resolved
Comment on lines +95 to +97
if (comment.body === finished) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks unreachable. Even if you comment twice for same SHA two different runs will be started if the automation hasn't posted a comment yet

Copy link
Member Author

Choose a reason for hiding this comment

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

This action is called from the test jobs created from a matrix, so there can be more than one. This is done to signal the original PR about the failure as early as possible.

Copy link
Member

Choose a reason for hiding this comment

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

this one deserves comment IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the workflow?

Copy link
Member

Choose a reason for hiding this comment

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

yes, in the body of this if block would make sense

.github/workflows/ci.yml Show resolved Hide resolved
@electrum electrum merged commit 1882c1d into trinodb:master Jun 22, 2023
@github-actions github-actions bot added this to the 420 milestone Jun 22, 2023
@nineinchnick nineinchnick deleted the ok-to-test-feedback branch July 5, 2023 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants