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

Implement rest handler for apiserver #111

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Implement rest handler for apiserver #111

merged 1 commit into from
Oct 12, 2022

Conversation

yuntanghsu
Copy link
Contributor

@yuntanghsu yuntanghsu commented Sep 16, 2022 •
edited
Loading

Add NetworkPolicyRecommnedation rest handler
Add unit-test for rest.go

Signed-off-by: Yun-Tang Hsu hsuy@vmware.com

@yuntanghsu yuntanghsu marked this pull request as draft September 16, 2022 00:15
yanjunz97
yanjunz97 reviewed Sep 16, 2022
View reviewed changes
pkg/querier/querier.go Outdated Show resolved Hide resolved
@yuntanghsu yuntanghsu marked this pull request as ready for review September 16, 2022 22:07
@yuntanghsu yuntanghsu marked this pull request as draft September 21, 2022 17:05
@yuntanghsu yuntanghsu force-pushed the pr_rest_handler branch 2 times, most recently from 9c3b732 to 7b1213f Compare September 23, 2022 00:04
@yuntanghsu yuntanghsu marked this pull request as ready for review September 23, 2022 19:48
@salv-orlando salv-orlando added this to the Theia v0.3 Release milestone Sep 28, 2022
yanjunz97
yanjunz97 reviewed Sep 28, 2022
View reviewed changes
pkg/apiserver/registry/intelligence/networkpolicyrecommendation/rest.go Outdated
Comment on lines 157 to 158
intelli.Status.ErrorCode = crd.Status.ErrorMsg
// todo: need to parse the error code
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
intelli.Status.ErrorCode = crd.Status.ErrorMsg
// todo: need to parse the error code
intelli.Status.ErrorMsg = crd.Status.ErrorMsg
// todo: need to parse the error code

I think you are still working on the error code parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.
I haven't come up with a good definition and use case of ErrorCode.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine to remove ErrorCode if it does not have a use case. Would like to get @wsquan171 's thought on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Originally the error code was added to facilitate API clients to programmatically react on different types of errors based on error code instead of error msg. For instance, a front end may prompt user differently if the error is by nature invalid input or cluster availability issue. error codes are in general better way to traceback issues in workflows compared to error msg.

For current CLI, since there's little internal retry, and propagating error msg back to stdout is good enough, the use of error code may not be that obvious. For long term serviceability considerations, I still think it's a good-to-have. cc @salv-orlando for more feedback on this.

pkg/apis/intelligence/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/apiserver/registry/intelligence/networkpolicyrecommendation/rest.go Outdated Show resolved Hide resolved
pkg/apiserver/registry/intelligence/networkpolicyrecommendation/rest.go Outdated Show resolved Hide resolved
@yuntanghsu yuntanghsu force-pushed the pr_rest_handler branch 3 times, most recently from e097e49 to 4b2d8a2 Compare September 29, 2022 17:11
wsquan171
wsquan171 reviewed Oct 6, 2022
View reviewed changes
Copy link
Contributor

@wsquan171 wsquan171 left a comment

Choose a reason for hiding this comment

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

overall lgtm. one nit.

pkg/apiserver/registry/intelligence/networkpolicyrecommendation/rest.go Outdated Show resolved Hide resolved
@yuntanghsu yuntanghsu force-pushed the pr_rest_handler branch 4 times, most recently from 60abff0 to 1f04430 Compare October 6, 2022 22:09
@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

@yuntanghsu yuntanghsu force-pushed the pr_rest_handler branch 2 times, most recently from f676a60 to 6a6231f Compare October 7, 2022 18:08
@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

@yuntanghsu yuntanghsu requested a review from wsquan171 October 10, 2022 21:02
wsquan171
wsquan171 approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@wsquan171 wsquan171 left a comment

Choose a reason for hiding this comment

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

LGTM

@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

@yuntanghsu yuntanghsu requested a review from yanjunz97 October 11, 2022 20:52
@yanjunz97
Copy link
Contributor

Overall LGTM, just think we need to have a decision on how to deal with the error code before merging.

@yuntanghsu yuntanghsu force-pushed the pr_rest_handler branch 2 times, most recently from 820119c to 86c3190 Compare October 12, 2022 18:38
Add NetworkPolicyRecommnedation restful handler
c81999a 
Add unit-test

Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
@yuntanghsu yuntanghsu merged commit dd818d5 into antrea-io:main Oct 12, 2022
@yuntanghsu yuntanghsu deleted the pr_rest_handler branch October 12, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wsquan171 wsquan171 wsquan171 approved these changes

@yanjunz97 yanjunz97 yanjunz97 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Theia v0.3 Release
Development

Successfully merging this pull request may close these issues.
4 participants
@yuntanghsu @yanjunz97 @wsquan171 @salv-orlando