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

fix: respect readOnly and writeOnly properties for required checks #130

Merged
merged 10 commits into from
Aug 9, 2022

Conversation

gssbzn
Copy link
Contributor

@gssbzn gssbzn commented Aug 3, 2022

Closes #126

Edit: the original approach was to filter only if not breaking changes but I though about it a bit better and I think it makes more sense to always filter readonly for a request type and write only for a response type since this is more aligned with the spec

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #130 (b38ca8e) into main (789538a) will increase coverage by 0.02%.
The diff coverage is 91.37%.

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   89.49%   89.51%   +0.02%     
==========================================
  Files          70       70              
  Lines        3949     4043      +94     
==========================================
+ Hits         3534     3619      +85     
- Misses        298      303       +5     
- Partials      117      121       +4     
Flag Coverage Δ
unittests 89.51% <91.37%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
diff/schema_diff.go 85.90% <87.17%> (-0.34%) ⬇️
diff/required_diff.go 95.89% <100.00%> (+3.99%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@@ -24,7 +24,7 @@ components:
required:
- id
- name
- nickname
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's no nickname property, if this was intentional need to address this at the test level

@@ -16,13 +16,15 @@ const (
)

func l(t *testing.T, v int) *openapi3.T {
t.Helper()
Copy link
Collaborator

Choose a reason for hiding this comment

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

:-)

@reuvenharrison
Copy link
Collaborator

reuvenharrison commented Aug 7, 2022

Hi Gustavo,
Your contribution is great!

In addition to the scenario you addressed (adding/deleting properties from/to the RequiredProperties list), I implemented solutions for a couple more use-cases:

  1. Adding/deleting properties
  2. Modifying read/write only flags for existing properties

It would be great if you could take a look and verify these use-cases too.

Thanks,
Reuven

@gssbzn
Copy link
Contributor Author

gssbzn commented Aug 9, 2022

@reuvenharrison thanks for looking into this, I see you pushed some changes to address the comments and I just merged main to resolve some conflicts, is there anything else you think I should look into before getting it merge?

@reuvenharrison reuvenharrison merged commit dd26a84 into Tufin:main Aug 9, 2022
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.

Readonly-required properties are being marked as breaking changes
3 participants