-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5569: [Build Failure] Spec Resync job is failing silently #2553
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
Conversation
…ust omitting the tests)
|
||
# PYTHON-5248 - Remove support for MongoDB 4.0 | ||
rm $PYMONGO/test/**/pre-42-*.json | ||
rm $PYMONGO/test/**/**/pre-42-*.json |
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 to remove files in test/discovery_and_monitoring/errors/pre-42-*.json
(I thought the **
was a recursive wildcard? but every time i ran the resync spec script those files would pop up so i added it explicitly?)
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 recommend using find
instead. I'm not a bash expert but the rules for ** aren't like in Path/.glob
.
Try: find /$PYMONGO /test -type f -name 'pre-42-*.json' -delete
Note that -f means that this isn't going to match directories.
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.
ooo good idea! I've replaced the two rm
s with the find
instead. Thanks~
@@ -1,5 +1,6 @@ | |||
#!/usr/bin/env bash | |||
# Run spec syncing script and create PR | |||
set -eu |
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 should make errors more visible to us
"single", | ||
+ "sharded" | ||
- "sharded", | ||
- "sharded-replicaset" |
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.
We already did this ticket but for whatever reason the spec repo still has this one instance of sharded-replicaset
so we're applying this patch to keep it out of our tests. (I've made a comment on the drivers ticket asking why its still in the spec repo)
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.
Apologies for the delay. I had a cluster of partial seizures last week so needed some rest.
|
||
# PYTHON-5248 - Remove support for MongoDB 4.0 | ||
rm $PYMONGO/test/**/pre-42-*.json | ||
rm $PYMONGO/test/**/**/pre-42-*.json |
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 recommend using find
instead. I'm not a bash expert but the rules for ** aren't like in Path/.glob
.
Try: find /$PYMONGO /test -type f -name 'pre-42-*.json' -delete
Note that -f means that this isn't going to match directories.
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.
Do these patches belong in this PR?
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.
The spec sync was failing and I figured it was easier to fix it in this PR than to put it off and fix it in the next spec sync PR. (running the spec sync script locally naturally produced these changes and since I was on greener build last week I decided to take the time to straighten them out.) I saw it as "catching" up on what the spec sync for the past few weeks should've been like and was hoping to make it easier for the next person on greener build.
If you prefer that they not be added in this PR, I'm happy to take them out.
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'm personally ok with this approach. I'm not a stickler and think velocity is often more important than paper trail.
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.
Same question on these new tests. I take it that the sync was failing and these just piled up?
All good, hope you go the rest you needed and that you're feeling better now! |
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.
LGTM!
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'm personally ok with this approach. I'm not a stickler and think velocity is often more important than paper trail.
Summary
Resync Spec task was silently failing and not generating any PRs.
https://jira.mongodb.org/browse/PYTHON-5569
Changes in this PR
set -eu
to the beginning of the bash file which should fail the task if the python script fails.Test Plan
I ran the script locally and verified desired behaviour. See below:
before my changes:
after my changes:
Screenshots (optional)
See above?
Checklist
Do not delete the items provided on this checklist
Checklist for Author
Checklist for Reviewer {@primary_reviewer}
Focus Areas for Reviewer
List any complex portion of code you believe needs additional scrutiny and explain why.
see comments :)