Support disabling polling of pluggable SCM materials created via config repositories

[peterrosell]

Issue Type

Bug report/Feature enhancement

Summary

I want to disable polling of materials of pipelines that are configured via a SCM Plugin and instead rely on web-hooks.

It works for manually created pipelines it's possible to set Repository polling behavior to Fetch updates to this repository only on webhook or manual trigger, but not when creating pipelines via a SCM plugin like gocd-yaml-config-plugin or gocd-json-config-plugin. When setting the auto_update flag in the config file it's silently ignored and always set to true in the internal data model.

Environment

GoCD Version: 23.2.0
YAML Configuration Plugin: 0.14.1
JSON Configuration Plugin: 0.6.0

Steps to Reproduce

1. Create a Pipeline in a repo and add it as a config repo.

format_version: 9
pipelines:
  pipe-test-1:
    group: TestGroup
    lock_behaviour: none

    materials:
      pipe-test-pr:
        plugin_configuration:
          id: github.pr
          version: 1
        options:
          url: [email protected]:peterrosell/test-repo.git
        auto_update: false
    stages:
      - test-pipe2:
          clean_workspace: true
          jobs:
            release:
              tasks:
                - exec:
                    command: /bin/bash
                    arguments:
                      - -c
                      - ls

2. Fetch the SCM config with curl and see that auto_update is set to true.

curl -s 'http://localhost:8153/go/api/admin/scms' -H 'Accept:application/vnd.go.cd.v4+json' | jq '.'

Expected Results

The auto_update flag should be set to false.

Actual Results

The auto_update flag is set to true.

Possible Fix

I've been digging around in the code and the flag doesn't exists in CRPluggableScmMaterial like it does in CRScmMaterial.
So by adding this property to CRPluggableScmMaterial and then make sure the flag propagates from PluggableSCMMaterialConfig to the CRPluggableScmMaterial.

Affected code in ConfigConverter.java here and here.

I have done some tests locally and got it to work so will create a PR to fix this issue.

[chadlwilson]

Thanks for the sleuthing here. It does seem that this should be possible to address, and may be an oversight from the way things are modeled with autoUpdate being owned in config by the top level SCM along with values like id/name etc.

gocd/config/config-api/src/main/java/com/thoughtworks/go/domain/scm/SCM.java

Lines 66 to 67 in 0f58107

	@ConfigAttribute(value = "autoUpdate", optional = true)
	private boolean autoUpdate = true;

However there is some potential complexity/mess here to consider due to the way SCMs are de-duplicated which we'd need to reconcile to ensure the semantics are the same as for the non-pluggable materials (equally deterministics or non-deterministic).

[MPV]

@chadlwilson Thanks for investigating this.
What do you think is the next step forward here?

[chadlwilson]

See the discussion at #11636 (comment) and below in particular. The extent of my thoughts are on the PR, and haven't got any further than that.

There's probably not really a next step without addressing the concerns on approach, which I'm not sure how to do right now (or put differently, have not had the time/energy/inclination to figure out).

The benefit appears somewhat small (given how long the config repo feature has been around and no-one highlighting this gap until now, that I can find!), and at the moment the effort/risk is relatively high, so we need to find a way to change that equation, which is rather difficult with the resources available.

TL;DR The current proposed approach in the PR has at least two problems

• can silently mutate configuration outside the config repository's scope, including materials that are not within scope of a given change, unintentionally affecting other pipelines.
• appears to mutate shared configuration objects in memory prior to a change being fully validated as safe to merge into the configuration, in a way that is not reverted/undone if the change later fails a validation constraint (this requires specific validation)

That's not really a solution I am comfortable with given the chance for unintended consequences that will be difficult for a user to reason about or debug.