-
Notifications
You must be signed in to change notification settings - Fork 6k
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][microcheck] manually add tests to microcheck #45400
Conversation
@MicroCheck //release:test_config //release:invalid_test Signed-off-by: can <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea that allowing users to pick which test to run is great. I am slightly against using git commit message though, as it often leads to weird-looking git commit messages merged into master.
can we use some other means? for example, asking users to manually trigger the pipeline with addtional env var? or maybe this is the time for us to build a GitHub app?
hmm can you elaborate more, when does this happen?
an option yes but it adds more frictions into the people workflow; i'm choosing the git commit message since that's already on the part of people existing workflow so it creates minimal overhead for folks |
i think users will prefer git commit messages way more than manually creating builds (i don't understand the github app enough to have an opinion); might be an overkill but we can ask a few OSS engineers if needed for them to choose the merge message is the github PR description so git commit message is a non-issue as well (understand that some repos merge message is git commit message but it should be a no issue for ray; also for those repos, people often have to self-massage the merge message since the git commit message format is inherently messy anyway) |
discussed offline, we agree to go with this solution and redo with another option if we see more than 3 mailformed git commit title caused by this from now until Sep 30th. |
for message in messages.decode().splitlines(): | ||
if message.startswith(MICROCHECK_COMMAND): | ||
tests = tests.union(message[len(MICROCHECK_COMMAND) :].strip().split(" ")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is decoding all messages? why not just the last one? I think we just need the last one?
is it possible to just check the git body, but not the title? then there is a lot less risks of having @microcheck
polluting the title, and much easier to meet the 0.3% goal.
and I am much more tolerant to having unrelated info in the commit message body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intentionally all messages yes, so that folks don't need to re-add tests again on every commit update
let me check how to get the body vs title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use format=%b
to get the git body only
- only check the git body Signed-off-by: can <[email protected]>
Allow PR owners to add tests to microcheck manually by adding a line with syntax
@microcheck //test_target_01 //test_target_02
to the git commit message. Thanks @stephanie-wang for the great idea.
Note that it's ok to provide an invalid target because
tester
will re-validate them withbazel query
.Test: