✨ Use RefUpdateRule in BranchProtection check #936
Conversation
azeemshaikh38
force-pushed
the
azeems/branches
branch
from
ff8e507
to
be02e11
Compare
|
Integration tests success for ff8e507186ca8ca13d9c1f0c98d27c37fa663eef |
azeemshaikh38
force-pushed
the
azeems/branches
branch
from
be02e11
to
e9720b3
Compare
|
Integration tests success for be02e11d8352b29903a1c0022023302c34c18528 |
|
Integration tests success for e9720b3b868f2b3603cccaa20e787da1100cf810 |
azeemshaikh38
force-pushed
the
azeems/branches
branch
from
e9720b3
to
743a2e9
Compare
|
Integration tests success for 743a2e97627b88d83633a899443819c5466cd5f6 |
| } | ||
| handler.data = new(branchesData) | ||
| if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil { | ||
| handler.errSetup = sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) |
There was a problem hiding this comment.
since the repo implementation returns a ErrScorecardInternal, we can assume in the checks that errors from any repo.* functions can be returned as-is. Correct?
There was a problem hiding this comment.
As of now, yes. But I think its best we don't keep this assumption. The sce.Err* errors should be returned by checks package. Internal packages like gtihubrepo should be free to return other internal errors.
There was a problem hiding this comment.
if we add ErrScorecardInternal in checks and the repo does too, we end up with multiple ErrScorecardInternal wrapping one another. Is there a better way to do it?
There was a problem hiding this comment.
Let's file an issue for cleaning this up because I think as of today, there is no clear guideline. IMO, only checks and pkg packages of Scorecard should return these Internal errors.
There was a problem hiding this comment.
there's one already #783
| @@ -51,8 +51,8 @@ var _ = Describe("E2E TEST:CodeReview", func() { | |||
| Error: nil, | |||
| Score: checker.MaxResultScore, | |||
| NumberOfWarn: 0, | |||
| NumberOfInfo: 2, | |||
| NumberOfDebug: 30, | |||
| NumberOfInfo: 0, | |||
There was a problem hiding this comment.
how about changing this tests to one of our test repos to avoid breaking? We can update it properly later.
There was a problem hiding this comment.
I would prefer leaving this in explicitly broken state so that we don't miss this when we are fixing e2e tests.
azeemshaikh38
force-pushed
the
azeems/branches
branch
from
743a2e9
to
0ff0823
Compare
|
Integration tests success for 0ff08238d7cada2a9920713c71083e4b611c77e2 |
azeemshaikh38
force-pushed
the
azeems/branches
branch
from
0ff0823
to
8fc9b04
Compare
|
Integration tests success for 8fc9b044669508e6ff2bd29c88c959c081c55f2a |
azeemshaikh38
force-pushed
the
azeems/branches
branch
from
8fc9b04
to
e20d93b
Compare
|
Integration tests failure for e20d93b9dc1b7698209e00db5182ffdc19345172 |
azeemshaikh38
force-pushed
the
azeems/branches
branch
from
e20d93b
to
8cc59ee
Compare
|
Integration tests success for 8cc59ee5d9598d33b0ba44d0334313c37128a89a |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Uses graphQL API
refUpdateRuleto access non-admin branch protection fields.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.