Add support to ignore api component errors #394
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 80.86% 80.97% +0.11%
==========================================
Files 208 209 +1
Lines 12232 12237 +5
==========================================
+ Hits 9891 9909 +18
+ Misses 1983 1971 -12
+ Partials 358 357 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
data/ignore-err-example.txt
Outdated
| @@ -1 +1,3 @@ | |||
| GET /api/{domain}/{project}/badges/security-score removed the success response with the status '200' | |||
| API removed the schema 'rules' [api-schema-removed] | |||
There was a problem hiding this comment.
I think we should prefix each line by its type, like this:
API GET /api/{domain}/{project}/badges/security-score removed the success response with the status '200'
components removed the schema 'rules' [api-schema-removed]
...
There was a problem hiding this comment.
not sure I'm following what you mean by type? here in this line we don't have the "httpMethod + endpoint" because it's a schema change, which is described in components
There was a problem hiding this comment.
ah, reread this and now i got it! yes that will be better
There was a problem hiding this comment.
update: I'm trying to update this but I seem to have found a bug in the unmarshalling/test. We are unmarshalling response as ApiChange, which leads to the wrong formatting. Checking if we can unmarshal w/ Change instead
There was a problem hiding this comment.
wont stress about trying to unmarshall different types as Changes is an interface, we can easilly just update this file w/ correct expectation (which is what we display)
checker/ignore.go
Outdated
|
|
||
| for errIndex, err := range errs { | ||
| if err.GetLevel() != level { | ||
| continue | ||
| } | ||
|
|
||
| typeOfChange := fmt.Sprintf("%T", err) | ||
| if typeOfChange != "checker.ComponentChange" { |
There was a problem hiding this comment.
Can we implement this through the Change interface instead of adding this if?
There was a problem hiding this comment.
yes, great idea, I'm adding the check for empty ignore paths within the MatchIgnore
| @@ -1 +1,4 @@ | |||
| GET /api/{domain}/{project}/badges/security-score removed the success response with the status '200' | |||
| in components removed the schema 'rules' | |||
There was a problem hiding this comment.
I suggest to require the keyword "components" to ignore a component.
| return false | ||
| func (c ComponentChange) MatchIgnore(ignorePath, ignoreLine string) bool { | ||
| return strings.Contains(ignoreLine, strings.ToLower(GetUncolorizedText(c))) && | ||
| strings.Contains(ignoreLine, "components") |
There was a problem hiding this comment.
I suggest to require the keyword "components" to ignore a change in components.
There was a problem hiding this comment.
I think we'll need to remove this based on the json output it wouldn't work:
[{"id":"api-schema-removed","text":"removed the schema 'SchemaValue'","level":3,"source":"Components"}
There was a problem hiding this comment.
I don't understand what you mean by "based on the json output it wouldn't work".
Can you explain the use case in more detail?
There was a problem hiding this comment.
Hi @blva.
I made a few minor changes.
Please review and if you agree I'll merge the PR.
Thanks,
Reuven
p.s., one more thing: we need to document this so people can use it
No description provided.