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

Add tests:hive automatically #10563

Merged

Conversation

findepi
Copy link
Member

@findepi findepi commented Jan 12, 2022

The labeler workflow is courtesy of Ashhar.

Co-authored-by: Ashhar Hasan [email protected]

@findepi
Copy link
Member Author

findepi commented Jan 12, 2022

Tested on my fork -- findepi#1 .

Copy link
Member

@homar homar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Thanks.

A question whether this works as expected.

@@ -0,0 +1,7 @@
# Pull Request Labeler Github Action Configuration: https://github.com/marketplace/actions/labeler
Copy link
Member

Choose a reason for hiding this comment

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

Note that labels get added "after" the PR gets created and hence the CI run won't see the labels most probably - can we test this? (subsequent pushes would see the labels though).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is what i expect. This automation doesn't free us from thinking.

However, oftentimes PRs do involve multiple pushes, so this may free us from doing the thing manually.

Copy link
Member

Choose a reason for hiding this comment

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

There's still one issue I see - a reviewer won't know whether the hive tests actually ran just by looking at presence of label.
I'm fine with merging this provided people know of the behaviour to expect from the label.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it is already discussed, what if we use workflow_run so that we could run this workflow could add labels and ci could use them ?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's still one issue I see - a reviewer won't know whether the hive tests actually ran just by looking at presence of label.

If you see a label added after the PR is created, you must not assume it was observed by the workflow run.

But, if there was a push since label was added, then you can assume it's observed

@findepi findepi requested a review from hashhar January 12, 2022 16:38
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % comments

.github/workflows/labeler.yml Show resolved Hide resolved
The labeler workflow is courtesy of Ashhar.

Co-authored-by: Ashhar Hasan <[email protected]>
@findepi findepi force-pushed the findepi/add-tests-hive-automatically-a8d83a branch from 9f3306f to 2cae5c9 Compare January 13, 2022 09:11
@findepi findepi merged commit 6e8e4c2 into trinodb:master Jan 13, 2022
@findepi findepi deleted the findepi/add-tests-hive-automatically-a8d83a branch January 13, 2022 09:12
@github-actions github-actions bot added this to the 369 milestone Jan 13, 2022
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