[JENKINS-60738] Fix global configuration submission from UI #347

Dohbedoh · 2023-05-02T11:29:45Z

Fixes JENKINS-60738 caused by changes introduced in #221.

Databinding of hookUrl is not working properly, per my understanding because the DataboundSetter setHookUrl(String hookUrl) accepts a String but the field is a URL. While adding a public void setHookUrl(URL hookUrl) might solve some of the problem, then there is yet another issue that disabling the Specify another hook URL for GitHub configuration checkbox does not remove the hook URL at all... that seems wrong.

So the propose fix:

handle the binding and the optional property state in the configure(StaplerRequest req, JSONObject json)
reactivate and adjusted the global configuration UI tests

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

julieheard

LGTM, manually tested and is working 🙂

jglick · 2023-05-03T18:31:43Z

While this sounds fine as a hotfix, the proper fix would be to delete all the overrides and special cases handling and fix the form to use normal databinding by convention.

jglick · 2023-05-03T18:33:29Z

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

            f.entry(title: _("Override Hook URL")) {
                g.blockWrapper {
                    f.optionalBlock(title: _("Specify another hook URL for GitHub configuration"),
+                            name: "isOverrideHookUrl",
                            inline: true,
                            checked: instance.isOverrideHookUrl()) {
                        f.entry(field: "hookUrl") {
-                            f.textbox(checkMethod: "post")
+                            f.textbox(checkMethod: "post", name: "hookUrl")
                        }
                    }
                }


For example all of this should just be like

<f:entry field="hookUrl"> <f:textbox label="Alternate hook URL for GitHub configuration"/> </f:entry>

which may be blank.

Oh I see. Remove all the optional block that is not necessary.. Yeah sounds good to me I can test this.

Would be more work for sure. The usual problem with this sort of fix is retaining compatibility for both XML settings and JCasC. Sometimes hacking up the JSON processing in the GUI logic is the only option since refactoring to a simpler structure would break something.

the thing that bother me is the type URL of the hookUrl... Because of that type, the value cannot actually be blank :( :

java.net.MalformedURLException: no protocol: at java.base/java.net.URL.<init>(URL.java:645) at java.base/java.net.URL.<init>(URL.java:541) at java.base/java.net.URL.<init>(URL.java:488) at org.kohsuke.stapler.Stapler$3.convert(Stapler.java:1141) Caused: org.apache.commons.beanutils.ConversionException: no protocol:

So it is not that simple to "reset" the hook URL because of the binding.. I thought about changing the type from URL to String but the existing public getter might be a problem: https://github.com/jenkinsci/github-plugin/blob/master/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java#L131-L141

Changing its signature could be a breaking change. Though it does not seem to be used outside the scope of that plugin repo: https://github.com/search?ref=simplesearch&type=code&q=user%3Ajenkinsci++getHookUrl.

BTW use permalinks for better display:

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

Lines 131 to 141 in 47b623a

/**

* @return hook url used as endpoint to search and write auto-managed hooks in GH

* @throws GHPluginConfigException if default jenkins url is malformed

*/

public URL getHookUrl() throws GHPluginConfigException {

if (hookUrl != null) {

return hookUrl;

} else {

return constructDefaultUrl();

}

}

jglick · 2023-05-03T18:34:35Z

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

+                    setHookUrl(null);
+                }
+                req.bindJSON(this, json);
+                clearRedundantCaches(configs);


Other than this method call, the whole configure method should not even need to be overridden.

The only way I found to simplify the data-binding and just do a req.bindJson is to change the signature of public URL getHookUrl() to public String getHookUrl(). That could be a breaking change. Example at 77cfb02.

So not sure if that would be acceptable ? cc @KostyaSha

better ask @jglick i forgot databinding nuances

That could be a breaking change. … So not sure if that would be acceptable ?

https://github.com/search?q=org%3Ajenkinsci%20getHookUrl&type=code does not look like it is used from other plugins.

jtnord · 2023-06-13T12:12:34Z

@KostyaSha are you ok with this as the active maintainer of this plugin?

jrtc27 · 2024-03-05T00:23:56Z

What's the status of this? I've just run into this with our own instance.

yahooguntu · 2024-03-06T23:09:04Z

I have the same issue; as a workaround I've been adding <hookUrl> directly to github-plugin-configuration.xml, but unfortunately it's cleared out when I save/apply System settings in the web UI.

jrtc27 · 2024-03-06T23:21:26Z

I have the same issue; as a workaround I've been adding <hookUrl> directly to github-plugin-configuration.xml, but unfortunately it's cleared out when I save/apply System settings in the web UI.

If you edit the config file and restart so it has the updated value in memory then the UI should show the correct setting and not clobber it on save.

franknarf8 · 2024-04-08T13:00:22Z

Can this be merged soon? I've just noticed this open PR after implementing the same fix and opening another PR : #375

Feel free to close the other PR

@KostyaSha @jglick

jglick · 2024-04-30T11:46:50Z

(I am not a maintainer)

[JENKINS-60738] Fix global configuration submission from UI

cfaa0c4

Dohbedoh force-pushed the JENKINS-60738 branch from 56e2723 to cfaa0c4 Compare May 2, 2023 11:31

[JENKINS-60738] Fix json binding for arrays

3a5d46f

Dohbedoh mentioned this pull request May 3, 2023

[JENKINS-71183] Fix Build actions anonymization enable UI check jenkinsci/support-core-plugin#462

Merged

6 tasks

julieheard approved these changes May 3, 2023

View reviewed changes

jglick reviewed May 3, 2023

View reviewed changes

Dohbedoh added 3 commits June 20, 2023 16:19

Merge branch 'master' into JENKINS-60738

a6ec398

Merge branch 'master' into JENKINS-60738

b14789f

Merge branch 'master' into JENKINS-60738

929dbe3

Merge branch 'master' into JENKINS-60738

e8952dd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-60738] Fix global configuration submission from UI #347

[JENKINS-60738] Fix global configuration submission from UI #347

Dohbedoh commented May 2, 2023 •

edited

julieheard left a comment

jglick commented May 3, 2023

jglick May 3, 2023

Dohbedoh May 4, 2023

jglick May 4, 2023

Dohbedoh May 5, 2023

jglick May 5, 2023

jglick May 3, 2023

Dohbedoh Jun 21, 2023 •

edited

KostyaSha Jun 21, 2023

jglick Nov 3, 2023

jtnord commented Jun 13, 2023

jrtc27 commented Mar 5, 2024

yahooguntu commented Mar 6, 2024

jrtc27 commented Mar 6, 2024

franknarf8 commented Apr 8, 2024

jglick commented Apr 30, 2024

	/**
	* @return hook url used as endpoint to search and write auto-managed hooks in GH
	* @throws GHPluginConfigException if default jenkins url is malformed
	*/
	public URL getHookUrl() throws GHPluginConfigException {
	if (hookUrl != null) {
	return hookUrl;
	} else {
	return constructDefaultUrl();
	}
	}

[JENKINS-60738] Fix global configuration submission from UI #347

Are you sure you want to change the base?

[JENKINS-60738] Fix global configuration submission from UI #347

Conversation

Dohbedoh commented May 2, 2023 • edited

julieheard left a comment

Choose a reason for hiding this comment

jglick commented May 3, 2023

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

Dohbedoh Jun 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord commented Jun 13, 2023

jrtc27 commented Mar 5, 2024

yahooguntu commented Mar 6, 2024

jrtc27 commented Mar 6, 2024

franknarf8 commented Apr 8, 2024

jglick commented Apr 30, 2024

Dohbedoh commented May 2, 2023 •

edited

Dohbedoh Jun 21, 2023 •

edited