[PFS 227] nil out entire branch #9944
Conversation
Zhang-Muyang
changed the title
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. |
FahadBSyed
requested review from
FahadBSyed
and removed request for
brendoncarroll
There was a problem hiding this comment.
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.
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.
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.
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. |
Zhang-Muyang
force-pushed
the
muyang/PFS-227-nilEntireBranch
branch
2 times, most recently
from
fbdb813
to
83c64c8
Compare
Zhang-Muyang
force-pushed
the
muyang/PFS-227-nilEntireBranch
branch
from
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.