-
Notifications
You must be signed in to change notification settings - Fork 155
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
Use relative links for objects within the same Confluence instance #477
Conversation
b099ff8
to
3d054b6
Compare
3d054b6
to
9331fe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my perspective 👍
} | ||
// Confluence supports relative links to reference other pages: | ||
// https://confluence.atlassian.com/doc/links-776656293.html | ||
linkPath := linkUrl.Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this always provide the relative path instead of the absolute path with baseurl now?
I think it would be good to have tests for this one, I'd like to avoid breaking things again. Would you be willing to write some?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mrueg,
Thanks a lot for reviewing!
Indeed, relative paths should always work. The problem last time was that, in the case of Confluence Cloud, baseURL
already contains a subpath (/wiki
) that should be prepended to those relative paths (Scenario 2 below).
In this case we take a more robust approach by forming the absolute URL first, then returning the relative path to make the page links independent of the hostname.
Scenario 1 - baseURL
doesn't contain a subpath (Confluence Server)
baseURL
:https://[my_server_hostname]>
pageLink
:/spaces/~spaceId/my_page
absoluteURL
:https://[my_server_hostname]>/spaces/~spaceId/my_page
relativePath
:/spaces/~spaceId/my_page
(equal topageLink
)
Scenario 2 - baseURL
contains a subpath (Confluence Cloud)
baseURL
:https://[company-name].atlassian.net/wiki
pageLink
:/spaces/~spaceId/my_page
absoluteURL
:https://[company-name].atlassian.net/wiki/spaces/~spaceId/my_page
relativePath
:/wiki/spaces/~spaceId/my_page
("/wiki" +pageLink
)
I thought about adding a separate function to isolate and test this functionality:
func getRelativePath(absoluteLink string) (string, error) {
linkUrl, err := url.Parse(absoluteLink)
if err != nil {
return "", karma.Format(err, "parse URL: %s", link)
}
return linkUrl.Path, nil
}
However, testing this function would be equivalent to testing the url
library (linkUrl.Path
).
In any case, I'm happy to write those tests if you think they'd provide any value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the further explanation. I think this looks good now. I got confused because I initially thought we'll only replace links in cloud. :)
Solves #456