-
Notifications
You must be signed in to change notification settings - Fork 567
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
[PFS 227] nil out entire branch #9944
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9944 +/- ##
==========================================
- Coverage 58.35% 58.25% -0.10%
==========================================
Files 614 614
Lines 75607 75675 +68
==========================================
- Hits 44117 44086 -31
- Misses 30935 31031 +96
- Partials 555 558 +3 ☔ View full report in Codecov by Sentry. |
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 only message type that is left to handle is FindCommitsResponse
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.
Changes to python sdk test files look good
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.
Looks good. It would be nice if this could be done as a validation rule, so that we can start validating other outputs. It would also be good to do a pass and see which code emits nil branches, and stop them from doing that. A dpanic in the interceptor when this happens would make that easy.
return nil, err | ||
} | ||
if b, ok := resp.(branchNillable); ok { | ||
b.NilBranch() |
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.
It would be nice if a non-nil branch logged at level dpanic instead. This would make tests that return non-nil branches fail, and we could fix everything at the source.
It would also be nice if we called ValidateAll on the way out, and implemented the nil branch requirement through validation rules. (Validation failure on output should just be a log.DPanic.)
var b1 = &Branch{Name: "dummy"} | ||
var c1 = &Commit{Branch: b1} | ||
c1.NilBranch() | ||
require.Nil(t, c1.Branch) |
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.
A better test would require.NoDiff
with an expected branch. This way you can be sure that NilBranch() doesn't also delete the Name field.
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 code changes look good to me.
You just need to fix the merge conflict on the MODULE.bazel.lock file.
@jrockway made some good points, I think they can be addressed in a follow-up PR.
Ah, these commit were efforts trying to rebase... But it turns out didn't solve the conflict. I'll try again to resolve the conflict. |
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
fbdb813
to
83c64c8
Compare
83c64c8
to
766c55e
Compare
This PR is to nil out branch field in responses. This is done in interceptors. Motivated by need to reduce confusion about relationship between Commits and Branches. Most of the changes in this PR are to fix tests. 90%+ of failed tests are due to nil pointer problem (branch field is nil). A common fix is: we resolve commit by its id instead of by its branch.
This PR is to nil out branch field in responses. This is done in interceptors.
Motivated by need to reduce confusion about relationship between Commits and Branches.
Most of the changes in this PR are to fix tests. 90%+ of failed tests are due to nil pointer problem (branch field is nil). A common fix is: we resolve commit by its id instead of by its branch.