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: remove extra slash when constructing member cluster URL if the apiEndpoint of the cluster object ends with a slash #5432

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

spiritNO1
Copy link
Contributor

@spiritNO1 spiritNO1 commented Aug 27, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #5228

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

karmada-aggregated-apiserver: User can append a "/" at the end when configuring the cluster's apiEndpoint.

@karmada-bot karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 27, 2024
@spiritNO1 spiritNO1 changed the title fix: cluster对象apiEndpoint以/结尾时构造member集群url多余一个/符号 fix: remove extra slash when constructing member cluster URL if the apiEndpoint of the cluster object ends with a slash Aug 27, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 31.16%. Comparing base (19d1146) to head (f5f351b).
Report is 23 commits behind head on master.

Files Patch % Lines
pkg/registry/cluster/storage/aggregate.go 66.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5432      +/-   ##
==========================================
+ Coverage   31.05%   31.16%   +0.10%     
==========================================
  Files         638      640       +2     
  Lines       44307    44414     +107     
==========================================
+ Hits        13760    13842      +82     
- Misses      29552    29569      +17     
- Partials      995     1003       +8     
Flag Coverage Δ
unittests 31.16% <66.66%> (+0.10%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@spiritNO1
Copy link
Contributor Author

/assign @chaunceyjiang

@spiritNO1 spiritNO1 removed their assignment Aug 27, 2024
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 28, 2024
… the apiEndpoint of the cluster object ends with a slash.

Signed-off-by: 圣朋 <[email protected]>
@XiShanYongYe-Chang
Copy link
Member

Hi @spiritNO1, have you tested it yet?

Copy link
Member

@chaunceyjiang chaunceyjiang left a comment

Choose a reason for hiding this comment

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

Thanks~

This is a great patch! I have already tested it in my local environment. It is working correctly.

 kubectl get  --raw /apis/cluster.karmada.io/v1alpha1/clusters/*/proxy/apis/apps/v1/namespaces/default/deployments/nginx

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2024
@chaunceyjiang
Copy link
Member

/kind bug

You need to write the release notes in the description of the PR.

@karmada-bot karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 28, 2024
@spiritNO1
Copy link
Contributor Author

Hi @spiritNO1, have you tested it yet?

Yes, modify the cluster.spec.apiEndpoint with / at the end, and not with / at the end.
Original code cannot work with / at the end, after fix it can work.

@spiritNO1
Copy link
Contributor Author

/kind bug

You need to write the release notes in the description of the PR.

done

@XiShanYongYe-Chang
Copy link
Member

done

How about add the component name in the release note:

karmada-aggregated-apiserver: xxx

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Thanks a lot
/approve

Hi @spiritNO1 would you like to help cherry-pick this patch to the previous release branch?

I created a tracked issue #5455

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XiShanYongYe-Chang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2024
@karmada-bot karmada-bot merged commit 47cea6a into karmada-io:master Aug 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when using cluster proxy
6 participants