[JENKINS-33228] Misc global config page improvements #112

kohsuke · 2016-03-08T05:10:34Z

See JENKINS-33228 for the context of this discussion.

This PR consists of a number of small changes to slightly improve the global configuration page.

GitHubServerConfig and especially its help text is still not really adequate for reuse by other plugins, but those requires further work.

But it occupies the precious real estate and pushes all the form controls to the right. Given the way global configuration page works, this affects all the form controls of the global config page, not just this plugin.

... especially so if this plugin is meant to be a common configuration point for all GitHub related plugins, ala JENKINS-33228.

... as this plugin is used to configure GitHub instances that other plugins will also use. description for repeatableHeteroProperty isn't all that useful anyway, because when multiple instances are configured, the description gets pushed far down and it's not clear where that text is associated with.

There's no nested structure in GitHubServerConfig, so the added descriptiveness is not being useful.

And "custom API URL" is a poor wording when it really refers to GitHub Enterprise

It's also API endpoint.

kohsuke · 2016-03-08T05:10:45Z

@reviewbybees

ghost · 2016-03-08T05:17:06Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

…t after And if someone is testing a setting, cache can be only harmful, so might as well disable that. This also prevents garbage from accumulating

jenkinsadmin · 2016-03-08T07:01:43Z

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

KostyaSha · 2016-03-08T08:13:35Z

src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java

@@ -171,7 +171,7 @@ public boolean configure(StaplerRequest req, JSONObject json) throws FormExcepti

    @Override
    public String getDisplayName() {
-        return "GitHub Plugin Configuration";
+        return "GitHub Configuration";


When you maintain different production installations with many plugins it extremely critical to know what plugin contributes what configuration 👎

And yes, i know about help buttons, but they may be broken by core like in different plugins today.

they may be broken by core

What is this referring to?

it [is] extremely critical to know what plugin contributes what configuration

But it is precisely the point of this PR to make this configuration be more readily applicable to multiple plugins, regardless of which plugin contributes it.

From the user perspective, they are configuring GitHub settings in Jenkins. If they need to troubleshoot why something is not working as expected, there are multiple techniques for doing that, such as enabling logging, searching known bug reports, etc. The link in help text may indeed be used to determine which plugin is defining a given configuration fragment, but this is not necessarily the source of a problem anyway.

Github plugin provides connection for plugins, it has own documentation that guides on how configure this connection. I.e. in pullrequest-plugin i have reference: 1) configure github-plugin 2) configure PR plugin.
Please remember that non enterprise usage it's not marketing feature where you want mask everything and resolve all issues via support. FOSS users should understand what and where should be configured.

I think we can review and bit update github-plugin documentation to indicate that it could be used by other plugins.Technically it has connection API + ancient push->GitSCM.poll() parts. Other plugins should also reference to github-plugin (i.e. if configured user should have additional permissions).

I'd also point out that most other plugins do not put "Plugin" nor "Configuration" in it. In fact I should have further removed "Configuration" from the subject line.

Yeah, "configuration" sounds bit redundant. Would it be possible to place some UI element that would show that this section comes from gh plugin, then i would be ok to remove "plugin" word?

this ok for me (it was here for historical reason)

At all, would it render help button if we place somewhere help.html?

KostyaSha · 2016-03-08T08:29:54Z

@kohsuke please do PRs from your forked repository, jenkinsci is full of your stalled branches (example https://github.com/jenkinsci/jenkins/branches/stale) and commits creating comments in JIRA.

Also for UI improvements please attach screenshot before and after.

This changes seems what i said before to @lanwen, but he decided to target on easier default UI. Let's wait his review.

KostyaSha · 2016-03-08T08:35:59Z

src/test/java/org/jenkinsci/plugins/github/internal/GitHubClientCacheCleanupTest.java

-                .doVerifyCredentials(github.serverConfig().getApiUrl(), credsId, 1);
+        GitHubServerConfig config = new GitHubServerConfig(credsId);
+        config.setApiUrl(github.serverConfig().getApiUrl());
+        config.setClientCacheSize(1);


@kohsuke maybe it would be easy for you to implement this https://issues.jenkins-ci.org/browse/JENKINS-32503 ?

jglick · 2016-03-08T15:00:38Z

please do PRs from your forked repository

Agreed, origin PRs should be reserved for cases where collaboration is expected.

As far as I can see, no other plugins put 'Configuration' either

kohsuke · 2016-03-08T18:09:34Z

Before:

After:

lanwen · 2016-03-08T18:20:00Z

src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy

@@ -5,15 +5,12 @@ import com.cloudbees.jenkins.GitHubPushTrigger
 def f = namespace(lib.FormTagLib);

 f.section(title: descriptor.displayName) {
-    f.entry(title: _("Servers configs with credentials to manage GitHub integrations"),
-            description: _("List of GitHub Servers to manage hooks, set commit statuses etc."),


think description is useful, as of some users confused what is this section for

In the interest of getting other changes in, if this is something you feel strongly I'm happy to give up, but you should see it when you have one or two GitHubServerConfig instances. This text will really look out of place. Existing help file should explain what this field does. In addition, if this plugin is to be better reused from elsewhere, the wording is incorrect, too.

Yes, github servers configuration part should be generified in UI and docs. But clicking help buttons is also not always convenient. Try open all collapsed items in configuration page to get review of configurations - you will tire clicking.

KostyaSha · 2016-03-08T18:30:30Z

@kohsuke thanks for screenshots

After:

I think it would make sense place test button under manage hooks as ideally it should also verify ability to manage hooks (that is not possible to check 🏧 because github-api doesn't expose headers AFAIR).

lanwen · 2016-03-08T18:49:59Z

src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy

            help: descriptor.getHelpFile()) {

        f.repeatableHeteroProperty(
                field: "configs",
-                hasHeader: "true",
-                addCaption: _("Add GitHub Server Config"),


Maybe just set to Add GitHub Server? As of its better for end-users than simple "Add" (it adds a userfriendly context)

I also don't like default "add" "delete" as they are not very useful on what they really do because of pure jelly form selection (it visible, but with all not aligned checkboxes not very convenient).

(Small reminder: verify documentation and screenshots if this changes would be merged.)

kohsuke · 2016-03-08T22:37:43Z

I think it would make sense place test button under manage hooks as ideally it should also verify ability to manage hooks (that is not possible to check 🏧 because github-api doesn't expose headers AFAIR).

When the plugin actually develops the capability to check it, please make that change. This is out of scope of this PR.

KostyaSha · 2016-03-08T22:40:34Z

This is out of scope of this PR.

According to commits description you reordering items. Changing later would require redoing screenshots. Placing now probably would also exclude bad aligned help buttons.

KostyaSha · 2016-03-08T22:45:11Z

src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy

            help: descriptor.getHelpFile()) {

        f.repeatableHeteroProperty(
                field: "configs",
                hasHeader: "true",
-                addCaption: _("Add GitHub Server Config"),
-                deleteCaption: _("Delete GitHub Server Config"))


Restore deleteCaptionif you restored addCaption ?

think add caption is enought

KostyaSha · 2016-03-08T22:46:28Z

FYI @kohsuke you may wait for both: my and @lanwen comments before doing changes.

lanwen · 2016-03-09T14:54:02Z

so, ok for me 👍
wait for a few more hours to additional reviews and will merge this

[JENKINS-33228] Misc global config page improvements

lanwen · 2016-03-10T10:41:24Z

Will release it as 1.18.0 together with #111 (after it will be completed)

amuniz · 2016-03-17T18:32:49Z

src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java

-     * @param customApiUrl true if optional block "Custom GH Api Url" checked in UI
-     */
-    @DataBoundSetter
-    public void setCustomApiUrl(boolean customApiUrl) {


This change broke backward compatibility, for example here.

See description of the field. This was used only for UI because of pure jelly form.

This was used only for UI because of pure jelly form.

It doesn't matter, it's public API.

hmmm, thanks for the report, will return that in a few hours.

…t public api see details in jenkinsci#112

kohsuke added 8 commits March 7, 2016 19:53

Nobody cares what plugin is being configured!

dc33199

... especially so if this plugin is meant to be a common configuration point for all GitHub related plugins, ala JENKINS-33228.

Long text is not useful here

198d6c3

There's no nested structure in GitHubServerConfig, so the added descriptiveness is not being useful.

optionalBlock that just has one field feels pointless

f2a02a4

And "custom API URL" is a poor wording when it really refers to GitHub Enterprise

Sensible order is endpoint,credentials, then other stuff

d4a13b3

It's not just credentials we are testing

fac0f8f

It's also API endpoint.

Everything here is clearly configuration, so this is redundant

bbcdc95

kohsuke force-pushed the config-improvements branch 2 times, most recently from 780988d to 96c8426 Compare March 8, 2016 05:28

kohsuke added 2 commits March 7, 2016 21:28

validate button can only work with fields that are defined before, no…

de4f7a3

…t after And if someone is testing a setting, cache can be only harmful, so might as well disable that. This also prevents garbage from accumulating

Help text updates

96c8426

KostyaSha reviewed Mar 8, 2016
View reviewed changes

Obviously everything here is configuration

aade2d4

As far as I can see, no other plugins put 'Configuration' either

lanwen reviewed Mar 8, 2016
View reviewed changes

kohsuke added 2 commits March 8, 2016 14:25

Removing these methods as requested in PR #112

48c754f

Mentioning megabytes as per requested in #112

c990995

Trying to make the reviewer happy

633ce4e

Unused imports

2e31640

KostyaSha reviewed Mar 8, 2016
View reviewed changes

lanwen added a commit that referenced this pull request Mar 10, 2016

Merge pull request #112 from jenkinsci/config-improvements

44a8781

[JENKINS-33228] Misc global config page improvements

lanwen merged commit 44a8781 into master Mar 10, 2016

lanwen deleted the config-improvements branch March 10, 2016 10:38

amuniz reviewed Mar 17, 2016
View reviewed changes

lanwen added a commit to lanwen/github-plugin that referenced this pull request Mar 17, 2016

revert setter for custom api url (bool) in GitHubServerConfig as of i…

a93e9c8

…t public api see details in jenkinsci#112

lanwen mentioned this pull request Mar 17, 2016

revert setter for custom api url (bool) in GitHubServerConfig as of i… #115

Merged

[JENKINS-33228] Misc global config page improvements #112

[JENKINS-33228] Misc global config page improvements #112

Conversation

kohsuke commented Mar 8, 2016

kohsuke commented Mar 8, 2016

ghost commented Mar 8, 2016

jenkinsadmin commented Mar 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KostyaSha commented Mar 8, 2016

Choose a reason for hiding this comment

jglick commented Mar 8, 2016

kohsuke commented Mar 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KostyaSha commented Mar 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohsuke commented Mar 8, 2016

KostyaSha commented Mar 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KostyaSha commented Mar 8, 2016

lanwen commented Mar 9, 2016

lanwen commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment