Skip to content
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

Add more cl checks #297

Merged
merged 18 commits into from
Jun 21, 2023
Merged

Add more cl checks #297

merged 18 commits into from
Jun 21, 2023

Conversation

@blva blva force-pushed the add-more-cl-checks branch from 9bd6dc2 to 5980baa Compare June 13, 2023 15:10
@blva blva force-pushed the add-more-cl-checks branch from 5980baa to 8376a5d Compare June 14, 2023 09:13
@blva blva marked this pull request as ready for review June 14, 2023 10:41
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2023

Codecov Report

Merging #297 (58aed89) into main (96be496) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
+ Coverage   78.73%   78.84%   +0.10%     
==========================================
  Files         164      164              
  Lines        9078     9122      +44     
==========================================
+ Hits         7148     7192      +44     
  Misses       1667     1667              
  Partials      263      263              
Flag Coverage Δ
unittests 78.84% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
checker/check-api-operation-id-updated.go 100.00% <100.00%> (ø)
checker/check-api-tag-updated.go 100.00% <100.00%> (ø)
...ecker/check-request-body-required-value-updated.go 100.00% <100.00%> (ø)
checker/check-response-status-updated.go 96.49% <100.00%> (ø)
checker/default_checks.go 91.26% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@tibulca tibulca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@reuvenharrison reuvenharrison self-requested a review June 16, 2023 15:06
Copy link
Collaborator

@reuvenharrison reuvenharrison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just letting you know that I am reviewing this, looks good so far...

verifyNonBreakingChangeIsChangelogEntry(t, d, osm, "api-tag-added")
}

// BC: adding an operation ID is not breaking with "api-operation-id-removed" check
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the omment to CL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, but let's discuss further at 9b30cba#r1233864561

@@ -82,11 +82,9 @@ These examples are automatically generated from unit tests.
[adding a new required property under AllOf in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L432)
[adding a new required read-only property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L486)
[adding a non-existent required property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L294)
[adding a tag is not breaking with "api-tag-removed" check](checker/checker_not_breaking_test.go?plain=1#L276)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reuvenharrison I was reserving the CLcomments to checker_test files, and keeping the ones in checker_not_breaking as BC so that we keep this doc up to date.

If you look at the entries below we are ending up duplicating what are changelog entries. LMK what you think and if I can revert this commit.

@blva blva requested a review from reuvenharrison June 19, 2023 10:33
@reuvenharrison reuvenharrison merged commit db4cd2e into Tufin:main Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants