Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support disabling polling of pluggable SCM materials created via config repositories #11635

Open
peterrosell opened this issue Jun 7, 2023 · 3 comments · May be fixed by #11636
Open

Support disabling polling of pluggable SCM materials created via config repositories #11635

peterrosell opened this issue Jun 7, 2023 · 3 comments · May be fixed by #11636
Labels
config repo materials plugins

Comments

@peterrosell
Copy link

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
  1. 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 chadlwilson changed the title Support disabling polling of materials of pipelines created via SCM Plugin Support disabling polling of pluggable SCM materials created via config repositories Jun 7, 2023
peterrosell added a commit to peterrosell/gocd that referenced this issue Jun 7, 2023
@peterrosell
support auto-update flag in pipelines from config repos
fcd096f 
Fix for gocd#11635.

Add support for the auto-update flag to be read and used
in materials of pipelines that is configured in config repos.
@peterrosell peterrosell linked a pull request Jun 7, 2023 that will close this issue
Draft
@chadlwilson
Copy link
Member

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).

@chadlwilson chadlwilson linked a pull request Jun 7, 2023 that will close this issue
support auto-update flag in pipelines from config repos #11636
Draft
@chadlwilson chadlwilson added this to the Release 23.2.0 milestone Jun 7, 2023
@MPV
Copy link

MPV commented Aug 15, 2023

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

@chadlwilson
Copy link
Member

chadlwilson commented Aug 15, 2023 •
edited
Loading

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.

@chadlwilson chadlwilson removed this from the Release 24.1.0 milestone Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
config repo materials plugins
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

support auto-update flag in pipelines from config repos peterrosell/gocd
3 participants
@MPV @peterrosell @chadlwilson