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][broker] update rest endpoint method names to avoid swagger conflict #20359

Merged
merged 1 commit into from
May 20, 2023

Conversation

pgier
Copy link
Contributor

@pgier pgier commented May 19, 2023

Renames two rest endpoint methods which were incorrectly named and cause an ID conflict in the generated swagger.

This change also skips the license check when generating the swagger spec file.

Motivation

Swagger generation uses the method/function name for the ID of the endpoint. This causes problems downstream when trying to generate admin client sources based on the swagger.

Modifications

Changed two method names related to namespace endpoints that didn't match the endpoint. This does not affect the rest endpoints, just the method names backing them.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: pgier#10

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 19, 2023
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Generally looks good. One cleanup comment below.

Copy link
Member

Choose a reason for hiding this comment

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

We can actually remove this file since the site repo is moved out to https://github.com/apache/pulsar-site.

And we write a similar script for generating swagger files: https://github.com/apache/pulsar-site/blob/main/tools/pytools/lib/execute/swagger_generator.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tisonkun Thanks, I have removed the gen-swagger.sh script.

Renames two rest endpoint methods which were incorrectly named and cause an ID
conflict in the generated swagger.

This change also removes the gen-swagger.sh script which is no longer used.

Signed-off-by: Paul Gier <paul.gier@datastax.com>
@pgier pgier force-pushed the fix-swagger-operation-ids branch from 5dbf726 to de866b7 Compare May 19, 2023 16:11
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you!

@tisonkun tisonkun added this to the 3.1.0 milestone May 20, 2023
@tisonkun tisonkun requested review from Technoboy- and nodece May 20, 2023 01:16
@nodece
Copy link
Member

nodece commented May 20, 2023

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

Codecov Report

Merging #20359 (de866b7) into master (2ebb379) will increase coverage by 35.93%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20359       +/-   ##
=============================================
+ Coverage     36.97%   72.90%   +35.93%     
- Complexity    12043    31759    +19716     
=============================================
  Files          1687     1864      +177     
  Lines        128760   138318     +9558     
  Branches      14003    15173     +1170     
=============================================
+ Hits          47612   100845    +53233     
+ Misses        74851    29465    -45386     
- Partials       6297     8008     +1711     
Flag Coverage Δ
inttests 24.22% <ø> (-0.10%) ⬇️
systests 25.03% <ø> (-0.05%) ⬇️
unittests 72.18% <ø> (+40.21%) ⬆️

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

Impacted Files Coverage Δ
.../org/apache/pulsar/broker/admin/v2/Namespaces.java 68.19% <ø> (+48.65%) ⬆️

... and 1430 files with indirect coverage changes

@nodece nodece merged commit eae4bd0 into apache:master May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants