expose JTH "jenkins.test.noSpaceInTmpDirs" property as a Maven property

[thomasgl-orange]

This is to go with jenkinsci/jenkins-test-harness#89, if it gets merged. I added a system property in JTH that some plugin might want to change, and this PR is to add the corresponding Maven property and passing it to surefire.

Note that there is no point in merging this unless the JTH PR gets merged and released first, and the JTH version in plugin-pom gets updated. My comment in the pom.xml mentions JTH version "2.33", this might need editing depending on what actually happens.

[KostyaSha]

1. they should fix it
2. they can set setting themselves.

[KostyaSha]

too much variables maven antipattern.
Doesn't look suitable to have separate variable

[daniel-beck]

The comment rubs me the wrong way. How is this not fixable?

I mean, sure, you may want (or need) a new parent POM for some reason, and want to release changes without addressing the new hoops the POM makes you jump through, but ultimately the goal is to get you to fix your broken plugin.

Disabling all the quality checks is definitely an anti-pattern, and I don't think we should make it this easy.

[thomasgl-orange]

@daniel-beck

How is this not fixable?

I don't know the details, but @MarkEWaite seemed to be in that case with the git plugin, which is why I've added the option to disable the thing in JTH in the first place (see comments in jenkinsci/jenkins-test-harness#89).

@KostyaSha

too much variables maven antipattern.

I've not made it explicit, but the reasoning for this Maven variable thing was to avoid plugins having to override the complete surefire's <systemProperties> configuration block when all they want is to add one property. Because this parent pom already defines other properties that they would have to copy/paste (ok, currently just one, hudson.udp=-1), and because the day other default properties get added here they would miss them.

But maybe it's over-engineering, I don't know...

[MarkEWaite]

Command line git (not the git plugin, but command line git) on Windows will fail credential operations with the git plugin if space characters are anywhere in the temporary directory path. The git plugin writes credential information (sensitive) into the temporary files, so it prefers to write those temporary files first into a temporary directory name derived from the workspace name, second into the system temporary directory. If the temporary directory name derived from the workspace name contains a space on Windows, the plugin falls back to the system temporary directory, with the resulting risk that sensitive information is being written to the system temporary directory.

if we were willing to abandon users running command line git versions before git 2.3, we could switch the git plugin to use GIT_SSH_COMMAND instead of temporary file. I'm not willing to abandon those users, since the standard git version delivered with CentOS 7 and CentOS 6 would be dropped based on that change.

Refer to the comments on the pull request that prompted this one.

[daniel-beck]

Not doing this does not break Git Plugin building. It just means that you'll have to set the upstream system property yourself.

To me, the question is about what's reasonable for plugins in general. And adding specific support for an escape hatch intended to catch plugin brokenness seems counterproductive.

Again, it's still possible to prevent the behavior introducing spaces. I've seen the upstream PR and think the system property is reasonable. The question is, to what degree do we want to specifically support doing so. For the vast majority of plugins, it should be considered not a good idea to opt out of this quality check (again -- Git is an exception as it already handles this problem AFAIUI).

[batmat]

@thomasgl-orange I'm planning to merge the upstream PR in the next days. This one is now conflicted.

Could you please:

• resolve the conflict
• File various PRs using a timestamped SNAPSHOT of here on some critical plugins? (Might be interesting to see how the git-plugin behaves in such case, or the pipeline-maven plugin cc @MarkEWaite @cyrille-leclerc).

Thanks!

[jglick]

it's still possible to prevent the behavior introducing spaces

By adding a Surefire configuration override to your plugin’s POM. That seems appropriately annoying to me.

For the particular case that @MarkEWaite mentioned, I would not even advocate doing this: I presume only a small minority of tests are actually performing clones of private repositories using non-SSH credentials. So these tests, when running on Windows, could use a distinct workspace location (e.g., normalWorkspace.replace(' ', '_')) with a comment explaining why they would otherwise fail. Or I presume that they fail based on this warning:

Some msysgit versions may fail credential related operations.

though #93 (comment) only says that

the plugin falls back to the system temporary directory, with the resulting risk that sensitive information is being written to the system temporary directory

which is presumably not a concern for test executions since the password is just something like changeme, not an actual secret. Or what am I missing?

[thomasgl-orange]

Could you please:

* resolve the conflict

Sure, done.

* File various PRs using a timestamped SNAPSHOT of here on some critical plugins? (Might be interesting to see how the git-plugin behaves in such case, or the pipeline-maven plugin cc @MarkEWaite @cyrille-leclerc).

I can try, but how do I get PR builds (first of jenkinsci/jenkins-test-harness#89, and then of this plugin-pom PR) deployed into the snapshots repository?

[batmat]

mvn clean deploy is accessible to every Jenkins developers for SNAPSHOT deployment. I possibly assumed you were developer on one plugin or so, which would provide you this access.

Anyway, for jenkins-test-harness I just released the 2.45 with your jenkinsci/jenkins-test-harness#89.

Please update this PR with this version of JTH, and I'll deploy a SNAPSHOT of plugin-pom.

Thanks!

[batmat]

Commit 9b2ec870c4a83bf0e4a214e4f4c399ab173c9de2 deployed as 3.33-20190110.085245-1 SNAPSHOT.

[batmat]

This is now conflicted FYI.

[thomasgl-orange]

@batmat

This is now conflicted FYI.

Thanks, I've updated the PR.

Note that, IMHO, this PR (with dep on JTH 2.45) should not be merged yet, not until we have a JTH 2.46 release with jenkinsci/jenkins-test-harness#122.

[batmat]

JTH released. Thanks!

[thomasgl-orange]

Thanks @batmat. I've updated this PR to get the new JTH version.

[jglick]

I think I would rather see a POM release that only upgraded JTH. If over the following weeks we find that there are a lot of difficult-to-solve problems with spaces in tests, then we would consider advertising an opt-out feature, but it seems premature here. Plugin maintainers who are determined to opt out can already do so by adding a surefire:test configuration block.

(And remember that once a property is added, it is tricky to remove, since there is no good way to mark a POM property as deprecated. You can simply cease to honor it, which would be pretty confusing; or you could introduce a profile activated by it which runs some mojo to fail the build, which is awkward and bloats the parent POM.)

[thomasgl-orange]

OK, I will close this PR then, and have submitted #153 instead for the JTH version bump. I agree that it was premature to introduce a new pom property before enough plugins actually need it.

The initial idea came because of anticipated issues with the git(-client) plugins, but actually, so far it looks likes git plugin now tests fine with JTH 2.46, and git-client requires only minor fixes.