Skip to content

[JENKINS-41891] Resource root url cleanup - #4294

Merged
timja merged 6 commits into
jenkinsci:masterfrom
jsoref:resource-root-url-cleanup
Nov 9, 2020
Merged

[JENKINS-41891] Resource root url cleanup#4294
timja merged 6 commits into
jenkinsci:masterfrom
jsoref:resource-root-url-cleanup

Conversation

@jsoref

@jsoref jsoref commented Oct 15, 2019

Copy link
Copy Markdown
Contributor

See JENKINS-41891.

Proposed changelog entries

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@daniel-beck

a rose by any other name may smell as sweet, but no one would know to buy it.
we make an exception for the label itself
@daniel-beck
daniel-beck self-requested a review October 16, 2019 00:18

@MRamonLeon MRamonLeon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why the removal of the root word?

@jsoref

jsoref commented Nov 21, 2019

Copy link
Copy Markdown
Contributor Author

It's the string that would make sense to normal people.

@MRamonLeon MRamonLeon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't feel confident to approve this as I'm not a native speaker. I only want to mention that the user here is the admin, as you know, so maybe this adjective (root) has some sense in this context. Anyway, you missed some resource root words, for example:

  • ...resource root URL candidate...
  • Can only set resource root URL...

Not sure if it's deliberated.

Thanks

@jsoref

jsoref commented Nov 22, 2019

Copy link
Copy Markdown
Contributor Author

There are four things:

  • Jenkins URL, conceptually this is the url a user conceives of for jenkins
  • Jenkins URLs -- any url within the space of the Jenkins URL
  • Resource root URL, the base for resource requests
  • Resource URLs -- any url served from the resource root.

The only person who cares about the resource root is the jenkins admin, and therefore using the word root is more ok. Also, there's a huge path between the resource root url and any resource url (due to its construction).

Everyone, including users and admins, deal with the Jenkins URL (it's a thing they visit directly).
They also will visit other Jenkins URLs which are direct children of the Jenkins URL. -- The resource root url is intentionally not a browseable thing, it behaves (to an end user) fairly differently from how the Jenkins URL behaves -- if you visit it directly, it just tells you to go away (although it could provide a slightly different response than a stapler 404).

@fcojfernandez
fcojfernandez requested a review from a team January 3, 2020 10:07
@res0nance

Copy link
Copy Markdown
Contributor

In cases where developer documentation is concerned i feel as though Jenkins Root Url is more appropriate jenkins does have an api for Jenkins::getRootUrl() so it should be consistent. For non-developer documentation, I'm not certain how else it is specified on jenkins.io

@jvz jvz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reads better now. I like your idea of dropping the word "root" here as it gets confusing what with two different root URL variables in the system that aren't exactly the same thing.

@daniel-beck

daniel-beck commented Jan 10, 2020

Copy link
Copy Markdown
Member

Interestingly, there are multiple locations where we call the "Jenkins URL" the "Jenkins root URL" (that are being cleaned up here), I got the impression that "Jenkins URL" was used consistently.

The benefit of going with this approach is that the term is simpler. The drawback is that there are many URLs pointing to Jenkins, and this is only the root (there is the term again) of that set of URLs. Just like the resource root URL is the root common to all individual resource URLs; so there could be confusion ("Which Jenkins URL? Which resource URL?").

I'm not convinced this direction is the better choice, but I'm not strongly against it either.

@daniel-beck
daniel-beck removed their request for review February 21, 2020 12:58

@oleg-nenashev oleg-nenashev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it is a reasonable improvement into the terminology. I do not feel strongly, but why not?
I would recommend to submit a pull request against https://jenkins.io/doc/book/glossary/ for "Jenkins URL" and "Resource Root URL" so that we can document the terms.

<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<f:section title="${%Serve resource files from another domain}">
<f:entry title="${%Resource root URL}" field="url">
<f:entry title="${%Resource Root URL}" field="url">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that it will break external localizations like Chinese Localization plugin. Heads-up to @LinuxSuRen

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about just improve it in the English properties?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@LinuxSuRen: No.

Would it help if I made a PR to update your thing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@LinuxSuRen: Imagine we misspelled "Chinese" as _("Cihnese") and that was the localization key. You could say "Why don't you just fix the English properties file to say Chinese?", but that would be wrong. And I'd be shocked if you actually suggested this.

What I'm doing here is effectively fixing the canonical spelling of terms. In part to prevent future confusion. One doesn't do that by wallpapering over it in a localization.

In a past life, I maintained a complete retranslation of a product into fixed English. At a certain point, the product management acknowledged that I was better than they were and agreed to try to accept my input.

If you need help updating your localization, I'm happy to make the changes on your side.

Beyond that, I am not remotely interested in stop energy here.

a(name: "resource-root-url")
form(method: "post", action: "${rootURL}/${my.url}/act") {
f.submit(name: 'redirect', value: _("Go to resource root URL configuration"))
f.submit(name: 'redirect', value: _("Configure resource root URL"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Impacts localizations if any

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about just improve it in the English properties?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@oleg-nenashev: There appear to be 0 in tree localizations.
@LinuxSuRen: I'm trying to change the meaning, so if someone actually did translate this message, I'd like them to think about the new key and try translating the new spirit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was wondering if you could file an issue on localization-zh-cn-plugin when this PR closed. Thanks.

@oleg-nenashev
oleg-nenashev requested a review from a team March 5, 2020 12:26
@oleg-nenashev oleg-nenashev added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Mar 5, 2020
@jsoref

jsoref commented Mar 5, 2020

Copy link
Copy Markdown
Contributor Author

@alecharp alecharp added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 3, 2020
@alecharp

alecharp commented Sep 3, 2020

Copy link
Copy Markdown
Member

@jsoref can you please review the conflicts? Thanks

@jsoref

jsoref commented Sep 4, 2020

Copy link
Copy Markdown
Contributor Author

@alecharp: I'm trying to address all of the items you tagged me on. hopefully i don't need to comment on each...

@alecharp alecharp removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 17, 2020
@varyvol

varyvol commented Sep 17, 2020

Copy link
Copy Markdown

@oleg-nenashev @LinuxSuRen could you please have a look to @jsoref 's answers? Thanks!

@rsandell rsandell added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 6, 2020
@timja
timja merged commit 6c6d787 into jenkinsci:master Nov 9, 2020
@timja
timja deleted the resource-root-url-cleanup branch November 9, 2020 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted

Projects

None yet

Development

Successfully merging this pull request may close these issues.