Drop support for git:// URLs

[basil]

Workarounds can sometimes be necessary in order to move a system forward without waiting an unreasonably long amount of time for consumers to be adjusted. Temporary workarounds can be tolerated for a short period of time, but the maintenance burden increases when those workarounds become permanent additions to the codebase. Such permanent workarounds increase the cognitive load required to understand the code.

The dynamic rewriting of git:// URLs to https:// is a workaround: if plugins properly declared their metadata with a working URL, there would be no need to do dynamic rewriting. The problem with this workaround is its scope: not only does it apply to all existing plugins (including plugins that already have valid URLs and do not need rewriting) but also all future plugins that are added to the managed set. In other words, with this workaround in place we might increase the amount of technical debt we have to bear without realizing it. This unbounded scope makes it more likely that the workaround will become permanent rather than temporary.

A setting like git config --global url.https://github.com/.insteadOf git://github.com/ (either applied automatically or advised via documentation) is even worse than the status quo. Not only is this a workaround for incorrect upstream metadata that allows us to introduce new plugins to the managed set with incorrect metadata without noticing it (like the status quo), but also it involves changing global Git configuration on the test system or developer system. Asking developers to make global changes for the sake of a particular repository is likely to cause trouble if different repositories have different global settings. The persistent and global nature of such a configuration can lead to drift, where a developer accumulates a large number of such changes and must then painstakingly recreate their old environment when moving to a new system. The fewer dependencies we have on global and persistent customizations, the better.

In contrast, this PR offers an improvement over the status quo by bounding the workaround to only the plugins that currently need it (because they define an invalid SCM URL). This is an improvement over the status quo because it means that new plugins cannot be added to the managed set unless their metadata is compliant. Thus the problem can only shrink, not grow. Over time, the problem will shrink to nothing, at which point the workaround can be removed, including its associated cognitive load and maintenance cost. Explicitly limiting the workaround to its smallest possible scope thus ensures that the workaround does not become permanent but remains temporary and can possibly be removed entirely, resulting in simpler code that is easier to understand and maintain.

To test this I have been running the BOM test suite in https://ci.jenkins.io/job/Tools/job/bom/job/PR-1826/ and limiting the workaround to apply to any plugins where I saw test failures.

[jtnord]

I don't think we should have any workarounds in the code, rather if the PCT can not work with git (its gone) or even SSH then it should just have git configured to use what it wants.

https://git-scm.com/docs/git-config#Documentation/git-config.txt-urlltbasegtinsteadOf

git config --global url.https://github.com/.insteadOf git://github.com/

[basil]

I addressed this in the review description.

[imonteroperez]

this PR offers an improvement over the status quo by bounding the workaround to only the plugins that currently need it (because they define an invalid SCM URL)

Is not possible to include this inside a dedicated hook(s) instead of inside the PluginCompatTester ?

[basil]

Is not possible to include this inside a dedicated hook(s) instead of inside the PluginCompatTester ?

Possibly, but I doubt the additional complexity would be worth the effort.

[imonteroperez]

Is not possible to include this inside a dedicated hook(s) instead of inside the PluginCompatTester ?

Possibly, but I doubt the additional complexity would be worth the effort.

No strong opinion here but I consider this is introducing some tech debt, I would prefer a hooks-based solution instead.

[jtnord]

missing several plugins that also require also workarounds.

apache-httpcomponents-client-4-api (release available but not yet picked up)
authentication-tokens PR
aws-global-configuration (release available, pending update)
blueocean-display-url PR
docker-commons
google-kubernetes-engine
google-metadata-plugin
google-oauth-plugin
kubernetes-credentials
node-iterator-api
oauth-credentials
pipeline-github-lib
pubsub-light
s3
suppress-stack-trace (never going to get a fix, to be excluded)
unique-id (release available pending update)
workflow-aggregator (release available, pending update)

[basil]

I consider this is introducing some tech debt

You appear not to have read the review description, which explains why this is decreasing rather than increasing the amount of technical debt. In any case, your consideration is not accompanied by its corresponding reasoning.

[basil]

missing several plugins that also require also workarounds

I have added the following plugins to the list:

• authentication-tokens
• blueocean-display-url
• google-kubernetes-engine
• google-metadata-plugin
• google-oauth-plugin
• kubernetes-credentials-provider
• node-iterator-api
• oauth-credentials
• pubsub-light
• s3

[imonteroperez]

I consider this is introducing some tech debt

You appear not to have read the review description, which explains why this is decreasing rather than increasing the amount of technical debt. In any case, your consideration is not accompanied by its corresponding reasoning.

Hardcoding 20 plugin entries for performing a connectionURL.replace in the main class of the PCT instead of doing it by a hook smells like introducing tech debt to me, but as mentioned no strong opinion, letting you decide the best option.

[basil]

aws-global-configuration (release available, pending update)

I have added aws-global-configuration to the list.

@jtnord Can you please retest on the CloudBees side and confirm we are ready to merge and release this?

[jtnord]

missing a workaround for kubernetes-credentals-plugin

[basil]

missing a workaround for kubernetes-plugin

Did you mean kubernetes-credentials-plugin? I have added kubernetes-credentials-plugin to the list.

@jtnord Can you please retest on the CloudBees side and confirm we are ready to merge and release this?

[jtnord]

missing a workaround for kubernetes-plugin

Did you mean kubernetes-credentials-plugin? I have added kubernetes-credentials-plugin to the list.

@jtnord Can you please retest on the CloudBees side and confirm we are ready to merge and release this?

yup my mistake, link was correct text was not.

[basil]

@jtnord Ready to go with this one? AFAICT the only issue in internal testing was with Stack Trace Suppression which we can easily suppress.

[jtnord]

@jtnord Ready to go with this one? AFAICT the only issue in internal testing was with Stack Trace Suppression which we can easily suppress.

I haven't been able to get a full build with abe9271 yet, but given the previous 2 failures kubernetes-credentials should be fixed and the other should be supressed in our build lets YOLO.