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.

Sign up for GitHub

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

Jump to bottom

Add check for API rule validation and fix type for Memcached test data #2113

Merged
merged 4 commits into from
Nov 4, 2019
Merged

Add check for API rule validation and fix type for Memcached test data #2113

merged 4 commits into from
Nov 4, 2019

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Oct 25, 2019 •
edited
Loading

Description of the change:

  • Add check in the tests to ensure that no issues related the API rule validations should be faced.
  • Fix type of spec for Memcached test data implementation.

Motivation for the change:

Closes #2114
Related to: #2042

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2019
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 25, 2019
@camilamacedo86 camilamacedo86 changed the title WIP : add tests for gen commands and fix openAPI validation issue WIP : Checking API rule violations fix openAPI validation issue Oct 25, 2019
@camilamacedo86 camilamacedo86 changed the title WIP : Checking API rule violations fix openAPI validation issue WIP : Checking API rule violations an fix mock data issue Oct 25, 2019
@camilamacedo86 camilamacedo86 changed the title WIP : Checking API rule violations an fix mock data issue WIP : Start to check API rule violations and fix mock data issue Oct 25, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 25, 2019
@camilamacedo86 camilamacedo86 changed the title WIP : Start to check API rule violations and fix mock data issue (fix test regards changes on 0.11) - Start to check API rule violations and fix mock data Oct 25, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2019
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 25, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2019
camilamacedo86
camilamacedo86 commented Oct 25, 2019
View reviewed changes
joelanford
joelanford reviewed Oct 25, 2019
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Same question I had here: operator-framework/operator-sdk-samples#82 (comment)

And a couple other questions about the necessity of some of the changes given the goal of the PR.

@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Oct 28, 2019 •
edited
Loading

HI @joelanford,

So the // +listType=set annotation currently has no impact on crd_test.go

It is not the point. The crd_test.go will do the test using a version which is outdated of the CRD since it is checking the test-framework which has not been updated and verified.

However, I will change this PR as you suggest no problem at all. Thank you for the input.

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 28, 2019
@camilamacedo86 camilamacedo86 changed the title (fix test regards changes on 0.11) - Start to check API rule violations and fix mock data add check for API rule validation and update mock data Oct 28, 2019
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2019
@camilamacedo86 camilamacedo86 changed the title add check for API rule validation and update mock data Add check for API rule validation and fix type for Memcached test data Oct 30, 2019
joelanford
joelanford previously approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM after addressing comments.

doc/user-guide.md Outdated
Nodes []string `json:"nodes"`
}
```

**NOTE** To know more about `+listType=set` see : https://godoc.org/k8s.io/kube-openapi/pkg/idl
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about making this note the similar to the one I suggested here?

NOTE: Comment directives, such as +listType=set, are necessary in certain situations to avoid API rule violations when generating OpenAPI files. See https://godoc.org/k8s.io/kube-openapi/pkg/idl to learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better :-)

camilamacedo86
camilamacedo86 commented Oct 31, 2019
View reviewed changes
doc/user-guide.md Outdated
Nodes []string `json:"nodes"`
}
```

**NOTE** To know more about `+listType=set` see : https://godoc.org/k8s.io/kube-openapi/pkg/idl
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better :-)

@camilamacedo86
Copy link
Contributor Author

Hi @joelanford,

The TODO was added as requested. Please, let us know if we could move forward with it now?

joelanford
joelanford reviewed Oct 31, 2019
View reviewed changes
@camilamacedo86
Copy link
Contributor Author

/test e2e-aws-subcommand

@camilamacedo86 @joelanford
Update doc/user-guide.md
4ddb69f 
Co-Authored-By: Joe Lanford <joe.lanford@gmail.com>
@camilamacedo86 camilamacedo86 added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2019
jmccormick2001
jmccormick2001 approved these changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@jmccormick2001 jmccormick2001 left a comment

Choose a reason for hiding this comment

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

lgtm

@camilamacedo86 camilamacedo86 merged commit e1c79ca into operator-framework:master Nov 4, 2019
@camilamacedo86 camilamacedo86 deleted the fix-test-gen branch November 4, 2019 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford left review comments

@jmccormick2001 jmccormick2001 jmccormick2001 approved these changes

@estroz estroz Awaiting requested review from estroz

@hasbro17 hasbro17 Awaiting requested review from hasbro17

@AlexNPavel AlexNPavel Awaiting requested review from AlexNPavel

@jmrodri jmrodri Awaiting requested review from jmrodri

@asmacdo asmacdo Awaiting requested review from asmacdo

Assignees
No one assigned
Labels
lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SDK tests are not validating open API rules
4 participants
@camilamacedo86 @joelanford @jmccormick2001 @openshift-ci-robot