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 http datastore fqdn address #1005

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

shjala
Copy link
Member

@shjala shjala commented Jul 24, 2024

In case the http datastore address has a port number, the datastore address should contain both the hostname and the port.

@shjala shjala requested a review from uncleDecart as a code owner July 24, 2024 09:39
Copy link
Member

@uncleDecart uncleDecart 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 @shjala ! Could you please add comment from your commit to the change, so it won't be gone if we rewrite commit? I think it's a useful comment for people :)

@shjala
Copy link
Member Author

shjala commented Jul 24, 2024

LGTM, thanks @shjala ! Could you please add comment from your commit to the change, so it won't be gone if we rewrite commit? I think it's a useful comment for people :)

I'm not sure what you mean, this is the commit message, should I add more to it?

Fix http datastore fqdn address
In case the http datastore address has a port number, the datastore address
should contain both the hostname and the port.

Signed-off-by: Shahriyar Jalayeri <[email protected]>

@shjala
Copy link
Member Author

shjala commented Jul 24, 2024

@uncleDecart got it, added.

Copy link
Collaborator

@giggsoff giggsoff left a comment

Choose a reason for hiding this comment

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

Please align the logic here.

// In case the http datastore address has a port number,
// the datastore address should contain both the hostname and the port.
if u.Port() != "" {
ds.Fqdn = fmt.Sprintf("%s:%s", ds.Fqdn, u.Port())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use u.Host instead in both cases (as full host:port notation), looks like it contains the port and u.Port() just extracts it from u.Host.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

In case the http datastore address has a port number, the datastore address
should contain both the hostname and the port.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
Copy link
Collaborator

@giggsoff giggsoff 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! Nice catch.

@milan-zededa milan-zededa merged commit 04badba into lf-edge:master Jul 24, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants