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

added check before getting the admin url #462

Closed
wants to merge 3 commits into from
Closed

added check before getting the admin url #462

wants to merge 3 commits into from

Conversation

bogi0704
Copy link

@bogi0704 bogi0704 commented Apr 5, 2022

Related issue: #461

This PR fixes a small bug which causes an index out of bounds error when calling it as a system org user. I have only added this code since there was no test for GetMetadata() itself.

@vmwclabot
Copy link
Member

@bogi0704, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@adambarreiro
Copy link
Collaborator

I'd bet that we have the same issue in other metadata functions that rely on getAdminVdcURL(vdcURL string), my suggestion here would be to fix that function. Maybe instead of:

func getAdminVdcURL(vdcURL string) string {
	return strings.Split(vdcURL, "/api/vdc/")[0] + "/api/admin/vdc/" + strings.Split(vdcURL, "/api/vdc/")[1]
}

We could do:

func getAdminVdcURL(vdcURL string) string {
	return strings.ReplaceAll(vdcURL, "/api/vdc/", "/api/admin/vdc/"))
}

If I'm not wrong this should be working in all cases. What are you thoughts on this?

@bogi0704
Copy link
Author

bogi0704 commented Apr 6, 2022

You are right, there are probably other Methods using getAdminVdcURL(vdcURL string) which would benefit from this fix. I have tested your suggestion of the fix and it also works when the vdcURL string already contains "/api/admin/vdc/". So nothing should break.

@vmwclabot
Copy link
Member

@bogi0704, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement.

@bogi0704
Copy link
Author

The CLA page shows this, so it should be fine now right?:

grafik

The Bot probably does not recognize that it is fine.

@adambarreiro
Copy link
Collaborator

adambarreiro commented Jun 6, 2022

Hi,
Actually we did a huge refactor of the metadata handling, planned for v3.7.0, in PR #473. In that PR we also included this fix (in a more generic fashion). If you don't mind I think we can close this one.

Thanks a lot!

@bogi0704 bogi0704 deleted the bogi0704-issue#461 branch June 7, 2022 09:48
@vmwclabot
Copy link
Member

@bogi0704, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants