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

api: fix duplicate gateway url issue #16464

Merged
merged 1 commit into from
Aug 27, 2023
Merged

api: fix duplicate gateway url issue #16464

merged 1 commit into from
Aug 27, 2023

Conversation

fuweid
Copy link
Member

@fuweid fuweid commented Aug 23, 2023

The HashKV should use /v3/maintenance/hashkv as url

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@fuweid
api: fix duplicate gateway url issue
f3121fb 
The HashKV should use `/v3/maintenance/hashkv` as url

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid
Copy link
Member Author

fuweid commented Aug 23, 2023

Will mark it ready after CI green

@ahrtr
Copy link
Member

ahrtr commented Aug 23, 2023 •
edited
Loading

This issue not being discovered in the first place means that it is NOT covered by the e2e test cases. We should try to add test case to cover all use cases. Let me take care of it in separate PR.

Link to #16454

@fuweid fuweid marked this pull request as ready for review August 23, 2023 13:35
ahrtr
ahrtr approved these changes Aug 23, 2023
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @fuweid

@fuweid
Copy link
Member Author

fuweid commented Aug 23, 2023

I was using gateway-v2 openapiv2 to find the duplicate issue since the tool returns error. I think the script/genproto.sh can cover it.

@ahrtr
Copy link
Member

ahrtr commented Aug 23, 2023

I was using gateway-v2 openapiv2 to find the duplicate issue since the tool returns error. I think the script/genproto.sh can cover it.

I mean the REST API isn't covered at all in the existing e2e test, otherwise, it should be discovered immediately.

@fuweid
Copy link
Member Author

fuweid commented Aug 23, 2023

I mean the REST API isn't covered at all in the existing e2e test

Yes. 😂 It needs time to add more test cases for it

jmhbnz
jmhbnz approved these changes Aug 23, 2023
View reviewed changes
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Nice work @fuweid. LGTM.

@ahrtr ahrtr mentioned this pull request Aug 24, 2023
Closed
@ahrtr
Copy link
Member

ahrtr commented Aug 27, 2023

Manually verified that both /v3/maintenance/hash and /v3/maintenance/hashkv work. Merging...

Will add e2e tests cases in separate PR.

@ahrtr ahrtr merged commit d09b8d4 into etcd-io:main Aug 27, 2023
27 checks passed
@ahrtr
Copy link
Member

ahrtr commented Aug 27, 2023

@fuweid can you please backport this PR to 3.5 as well? thx

@fuweid fuweid deleted the fix-grpc-gateway-duplicate-path branch August 28, 2023 02:32
@fuweid fuweid mentioned this pull request Aug 28, 2023
Merged
@serathius serathius mentioned this pull request Oct 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmhbnz jmhbnz jmhbnz approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fuweid @ahrtr @jmhbnz