[JENKINS-50164] Remove UI option to modify WS and builds locations #3364

batmat · 2018-03-22T11:21:45Z

Introduce system properties in lieu of the previous way using the UI for this.
Still leave the UI in readonly mode to ease checking configuration for admins.

Tests have been written to check using those system properties would work, even on an existing setup.

A warning log is now outputted when these system properties are used and changing the previous value, so that not only the help text warns about the fact there's no data migration.

See JENKINS-50164.

TODO before/just after merge:

* [x] document the new behavior, e.g. on the wiki or in the Jenkins handbook. This should be ready by the time the weekly changelog is published. Requested by @daniel-beck jenkins-infra/jenkins.io#1510

Update the sysprop standard page with new syprops

Proposed changelog entries

JENKINS-50164: Removed the option to modify Build Record Root Directory and Workspace Root Directory from the User Interface directly. Those are now only modifiable using system properties, refer to the standard page for these values TODO: write docs there once/if merged. Remember that no data migration is handled when using those options on an existing setup.

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

@reviewbybees


        [JENKINS-50164] Remove UI option to modify WS and builds locations

Test (almost) only: checking the previous behaviour is preserved, and the system property is accounted for when set.


        [JENKINS-50164] Remove UI option to modify WS and builds locations

Add necessary code to fix tests, and make the UI readonly.

jglick · 2018-03-22T13:50:01Z

core/src/main/java/jenkins/model/Jenkins.java

+        if (newBuildsDir != null && !buildsDir.equals(newBuildsDir)) {
+            LOGGER.log(Level.WARNING, "Changing builds directories from {0} to {1}. Beware that no automated data migration will occur.",
+                       new String[]{buildsDir, newBuildsDir});
+            buildsDir = newBuildsDir;


And need to save(), I suspect.

jglick · 2018-03-22T13:50:01Z

core/src/main/java/jenkins/model/Jenkins.java

+    private static final String OLD_DEFAULT_WORKSPACES_DIR = "${ITEM_ROOTDIR}/" + WORKSPACE_DIRNAME;
+
+    /**
+     * Default value for the workspace's directories layout.


@see #workspaceDir

done, see below

jglick · 2018-03-22T13:50:01Z

core/src/main/java/jenkins/model/Jenkins.java

+     * System property name to set {@link #buildsDir}.
+     * @see #getRawBuildsDir()
+     */
+    public static final String BUILDS_DIR_PROP = Jenkins.class.getName() + ".BUILDS_DIR";


I do not think these constants need to be public—we do not expect plugin Java code to refer to them.

jglick · 2018-03-22T13:50:01Z

core/src/main/resources/jenkins/model/Jenkins/configure.jelly

@@ -40,11 +40,11 @@ THE SOFTWARE.
        ${it.rootDir}
      </f:entry>
      <f:advanced>
-        <f:entry field="rawWorkspaceDir" title="${%Workspace Root Directory}">
-          <f:textbox/>
+        <f:entry field="rawWorkspaceDir" title="${%Workspace Root Directory}" >


stray space

jglick · 2018-03-22T13:50:01Z

core/src/main/resources/jenkins/model/Jenkins/configure.jelly

        </f:entry>
        <f:entry field="rawBuildsDir" title="${%Build Record Root Directory}">
-          <f:textbox/>
+          <f:textbox readonly="readonly"/>


Setting the readonly attribute will block a web browser from editing this field, but doConfigSubmit is continuing to accept modifications from HTTP posts! I think you want to delete those two lines as well.

I would really just suggest deleting this advanced block, and print a warning to the system log if there is an override which is not matched by the system property.

eeeek. Nice catch. I had deleted the associated lines in doConfigSubmit in #3360 but did a bad manipulation here and reverted this.

About the warning, I would like to avoid having it printed each time as WARNING, especially for instance since we'll use this in Essentials. Though I agree that logging as INFO when the value has not changed would serve the purpose I had here with keeping the UI to check the values.

done it all

jglick · 2018-03-22T13:50:01Z

test/src/test/java/jenkins/model/JenkinsInitializationTest.java

+    @Test
+    public void buildsDir() throws Exception {
+        loggerRule.record(Jenkins.class, Level.WARNING).capture(10);
+        story.addStep(new Statement() {


Use then, which takes a SAM that can be implemented via lambda.


        Make package visible to avoid public usage


        workspaceDir & buildDir not modifiable anymore from the UI!


        Remove the whole UI for buildsDir and workspaceDir


        Lambda-ify


        Better javadoc


        Log always as INFO minimum, WARNING if values were changed


        save() if fields were touched

batmat · 2018-03-22T16:28:09Z

@jglick I believe I addressed all your comments.

Only thing not done yet is checking how it works with https://issues.jenkins-ci.org/browse/JENKINS-21942 as requested in https://issues.jenkins-ci.org/browse/JENKINS-50164?focusedCommentId=332902&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-332902


        Move and refactor test as now the UI is not anymore

batmat · 2018-03-23T16:49:35Z

Build succeeded, reviews are welcome. Thanks!

jtnord · 2018-03-26T09:06:11Z

I am sceptical about this - esp because of a JIRA I am currently trying to track down - and also as this should be managed as CaC and not a System property.

Because as was said to me on a different PR No NOT another randomly hidden system property that alters behaviour / state.

daniel-beck · 2018-03-26T09:25:13Z

@jtnord What alternative to having a UI option (which clearly leads to broken behavior) do you propose?

jtnord · 2018-03-26T09:48:51Z

@daniel-beck

also as this should be managed as CaC and not a System property.

The UI option can lead to broken behaviour just as much as a System property.
At least in the UI you can do FormValidation such that they can be warned before they submit...

daniel-beck · 2018-03-26T09:59:07Z

@jtnord

The UI option can lead to broken behaviour just as much as a System property.

Not really, as it cannot easily be changed while Jenkins is running (and I'm trying to prevent CasC doing something dumb in this comment). Pulling the rug out from under builds in memory is pretty much unique to the UI option. (Also, no sane way around changing it via the UI, unless you really like XML).

jtnord · 2018-03-26T10:06:37Z

core/src/main/java/jenkins/model/Jenkins.java

+        }
+
+        if (mustSave) {
+            save();


seems scary as this is being called in a thread that is performing a load

Recommended by @jglick in #3364 (comment)

I'm not totally sure myself of the criticality of the risk, if there is one.

jtnord · 2018-03-26T10:21:32Z

test/src/test/java/jenkins/model/JenkinsBuildsAndWorkspacesDirectoriesTest.java

+ *
+ * @since TODO
+ */
+public class JenkinsBuildsAndWorkspacesDirectoriesTest {


Seems to be missing a test for the following:

Custom build/workspace are set in existing Jenkins config (from version without this change)

Jenkins is upgraded

Jenkins config is saved

configuration should not be reset - existing custom values should be retained until they set the system property

Because otherwise people are going to not do that step and mess things up.

And this implies there is no way to go back to default - once you have set the system property if you want to revert to the defaults you need to know what they are and set the system property to it (removing the system property should not change things)...

daniel-beck · 2018-03-26T10:27:19Z

It is unclear why #3360 was closed instead of updated, but given @jtnord's comments it seems quite a few of the review comments there are still relevant here.

jtnord · 2018-03-26T10:28:12Z

By broken I mean a user setting BUILD_ROOT=ITEM_ROOT. JENKINS-50335

jtnord · 2018-04-17T11:00:01Z

core/src/main/java/jenkins/model/Jenkins.java

+    /*private*/ static void checkRawBuildsDir(String value) throws InvalidBuildsDir {
+
+        // do essentially what expandVariablesForDirectory does, without an Item
+        String replacedValue = expandVariablesForDirectory(value,


this method is utterly confusing

jtnord · 2018-04-17T11:00:38Z

core/src/main/java/jenkins/model/Jenkins.java

+    }
+
+    /**
+     * @param value


useless javadoc. what is it checking for - embedding bitcoins?

jtnord · 2018-04-17T11:05:12Z

test/src/test/java/jenkins/model/JenkinsBuildsAndWorkspacesDirectoriesTest.java

+    public void badValueForBuildsDir() {
+        story.then((rule) -> {
+            final List<String> badValues = Arrays.asList(
+                    "blah",


so there are no bad values for windows?? this seems to be unix specific without an assume and may fail but likely not for the expected reason.

jtnord · 2018-04-17T11:08:02Z

test/src/test/java/jenkins/model/JenkinsBuildsAndWorkspacesDirectoriesTest.java

+    @Test
+    public void fromPreviousCustomSetup() {
+
+        assumeFalse(Functions.isWindows()); // Default Windows lifecycle does not support restart.


NIT: it is much nicer to use assumeFalse(" Default Windows lifecycle does not support restart.", Function.isWindows()) as then the reason for the skipped test will be reported.

jtnord

Seems ok apart from the checkRawBuildsDir method which is thoroughly confusing to me.


        Update javadoc & clarify parameter name


        Make the potential skip reason bubble up

batmat · 2018-04-20T12:30:24Z

I fixed some of your comments @jtnord. Thanks for the review!

To /all I plan to merge this before the next release, i.e. on Sunday max if nobody objects in the meantime.

Thanks!

jglick

Not sure I understand all the implications, but removing a UI option is generally a good thing, and I am assuming you are testing this.

jglick · 2018-04-20T16:55:36Z

core/src/main/resources/jenkins/model/Jenkins/configure.jelly

-        <f:entry field="rawBuildsDir" title="${%Build Record Root Directory}">
-          <f:textbox/>
-        </f:entry>
-      </f:advanced>


IMO: users should not be customizing this. If they do, they presumably have some reason and will be able to figure out example paths for themselves. If they are not customizing this, then they should have no reason to care. And the workspace root is only relevant for on-master builds, which you should never do to begin with.

jglick · 2018-04-20T16:55:36Z

test/src/test/java/jenkins/model/JenkinsBuildsAndWorkspacesDirectoriesTest.java

+ * So this test class uses a {@link RestartableJenkinsRule} to check the behaviour of this sysprop being
+ * present or not between two restarts.
+ *
+ * @since TODO


No @since tag needed on a test.

jglick · 2018-04-20T16:55:36Z

test/src/test/java/jenkins/model/JenkinsBuildsAndWorkspacesDirectoriesTest.java

+        );
+
+        story.then(steps -> {
+            assertTrue(story.j.getInstance().isDefaultBuildDir());


The lambda takes a parameter. And getInstance is static. Conventionally

story.then(r -> { assertTrue(r.jenkins.isDefaultBuildDir());

daniel-beck · 2018-04-21T11:45:58Z

On-holding this until the following is addressed (or at least gets a reasonable response):

Since we're removing documentation from Jenkins, make sure to document the new behavior, e.g. on the wiki or in the Jenkins handbook. This should be ready by the time the weekly changelog is published.

Also, make sure the PR comment and changelog message are updated.

batmat · 2018-04-22T13:42:28Z

Working on the on-holding reason now, hopefully to propose something very soon.

To loop back with a discussion on IRC with Daniel, I'm generally against squashing. So please just do not squash this one if you merge it. Thanks!

batmat · 2018-04-23T09:45:36Z

Just filed the docs PR linked above to un-on-hold this. Please review so that we can move forward here.

Thanks!

batmat · 2018-04-24T08:39:40Z

Updated https://wiki.jenkins.io/display/JENKINS/Features+controlled+by+system+properties?blah=eeek

batmat · 2018-04-24T09:28:55Z

Going to merge this in the next 10 minutes if nobody objects. The PR was already approved, and I've fixed the reason for putting it On-Hold.

batmat · 2018-04-24T09:30:26Z

For the record, the PR was built green previously. The current failure seems like an infra issue.


        Adjust sysprops names to JENKINS-50164 merge in core

Cf. jenkinsci/jenkins#3364


        Adjust sysprops names to JENKINS-50164 merge in core

Cf. jenkinsci/jenkins#3364 Manually tested for now, JENKINS-50940 will have the test automation for this.

KostyaSha · 2019-10-08T01:44:19Z

Am i correctly understand:
1 Jenkins.java: private static final String DEFAULT_BUILDS_DIR = "${ITEM_ROOTDIR}/builds"; is a default value
2 Then JenkinsRule calls Jenkins.loadTasks() -> Jenkins.loadConfig();,
3 by default config.xml is missing in tests, so checkRawBuildsDir(buildsDir) gets checkRawBuildsDir("${ITEM_ROOTDIR}/builds")
4. which expands to .cut../jobs/doCheckRawBuildsDir-Marker$foo/builds and fails because it's not an absolute path?

Or when empty Jenkins is running does it hit into wrong default by default?

KostyaSha · 2019-10-08T13:41:54Z

@jglick @batmat @daniel-beck @oleg-nenashev ?

KostyaSha · 2019-10-08T13:54:18Z

test/src/test/java/jenkins/model/JenkinsBuildsAndWorkspacesDirectoriesTest.java

+                    "$JENKINS_HOME/foo/$ITEM_FULL_NAME",
+                    "${ITEM_ROOTDIR}/builds");
+
+            for (String goodValue : badValues) {


so is it good or bad value/values?

jglick · 2019-10-08T14:25:02Z

when empty Jenkins is running does it hit into wrong default

JENKINS-21942

[JENKINS-50164] Remove UI option to modify WS and builds locations

Loading status checks…

7961bad

Test (almost) only: checking the previous behaviour is preserved, and the system property is accounted for when set.

batmat added the work-in-progress label Mar 22, 2018

[JENKINS-50164] Remove UI option to modify WS and builds locations

Loading status checks…

cce49e7

Add necessary code to fix tests, and make the UI readonly.

batmat mentioned this pull request Mar 22, 2018

[JENKINS-50164] Remove UI option to modify WS and builds locations #3360

Closed

4 of 4 tasks complete

batmat requested a review from daniel-beck Mar 22, 2018

batmat removed the work-in-progress label Mar 22, 2018

batmat changed the title ~~WIP [JENKINS-50164] Remove UI option to modify WS and builds locations~~ [JENKINS-50164] Remove UI option to modify WS and builds locations Mar 22, 2018

jglick reviewed Mar 22, 2018

View changes

batmat added 7 commits Mar 22, 2018

Make package visible to avoid public usage

9973430

workspaceDir & buildDir not modifiable anymore from the UI!

fa9326c

Remove the whole UI for buildsDir and workspaceDir

819aeeb

Lambda-ify

34bff66

Better javadoc

Loading status checks…

770c70a

Log always as INFO minimum, WARNING if values were changed

Loading status checks…

36fe239

save() if fields were touched

Loading status checks…

32bac4b

batmat requested a review from jglick Mar 22, 2018

batmat added the work-in-progress label Mar 22, 2018

Move and refactor test as now the UI is not anymore

Loading status checks…

3d9b6f3

batmat added needs-more-reviews and removed work-in-progress labels Mar 23, 2018

jtnord reviewed Mar 26, 2018

View changes

jtnord reviewed Apr 17, 2018

View changes

batmat added 2 commits Apr 20, 2018

Update javadoc & clarify parameter name

d8ef16e

Make the potential skip reason bubble up

Loading status checks…

84e423d

batmat added ready-for-merge and removed needs-more-reviews labels Apr 20, 2018

jglick approved these changes Apr 20, 2018

View changes

daniel-beck added the on-hold label Apr 21, 2018

batmat mentioned this pull request Apr 23, 2018

[JENKINS-50164] Document builds location configuration #1510

Merged

batmat removed the on-hold label Apr 24, 2018

batmat merged commit a363f2e into jenkinsci:master Apr 24, 2018
1 check failed

1 check failed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details

batmat deleted the batmat:JENKINS-50164 branch Apr 24, 2018

batmat added a commit to batmat/evergreen that referenced this pull request Apr 27, 2018

Adjust sysprops names to JENKINS-50164 merge in core

5c6c013

Cf. jenkinsci/jenkins#3364

batmat added a commit to batmat/evergreen that referenced this pull request Apr 27, 2018

Adjust sysprops names to JENKINS-50164 merge in core

2ce37fa

Cf. jenkinsci/jenkins#3364 Manually tested for now, JENKINS-50940 will have the test automation for this.

batmat mentioned this pull request Apr 27, 2018

Adjust sysprops names to JENKINS-50164/JEP-302 merge in core #65

Merged

KostyaSha reviewed Oct 8, 2019

View changes

jenkinsci / jenkins

Sponsor jenkinsci/jenkins

Join GitHub today

[JENKINS-50164] Remove UI option to modify WS and builds locations #3364

[JENKINS-50164] Remove UI option to modify WS and builds locations #3364

Conversation

batmat commented Mar 22, 2018 • edited

TODO before/just after merge:

Proposed changelog entries

Submitter checklist

Desired reviewers

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

batmat commented Mar 22, 2018

batmat commented Mar 23, 2018

jtnord commented Mar 26, 2018

daniel-beck commented Mar 26, 2018

jtnord commented Mar 26, 2018

daniel-beck commented Mar 26, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jtnord Mar 26, 2018 • edited

daniel-beck commented Mar 26, 2018

jtnord commented Mar 26, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jtnord left a comment

batmat commented Apr 20, 2018

jglick left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

daniel-beck commented Apr 21, 2018

batmat commented Apr 22, 2018

batmat commented Apr 23, 2018 • edited

batmat commented Apr 24, 2018

batmat commented Apr 24, 2018

batmat commented Apr 24, 2018

KostyaSha commented Oct 8, 2019 • edited

KostyaSha commented Oct 8, 2019

This comment has been minimized.

KostyaSha Oct 8, 2019 • edited

jglick commented Oct 8, 2019

batmat commented Mar 22, 2018 •

edited

jtnord Mar 26, 2018 •

edited

batmat commented Apr 23, 2018 •

edited

KostyaSha commented Oct 8, 2019 •

edited

KostyaSha Oct 8, 2019 •

edited