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 auto-update flag in pipelines from config repos #11636

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

support auto-update flag in pipelines from config repos #11636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fcd096f
support auto-update flag in pipelines from config repos
peterrosell Jun 7, 2023
bc229cb
Merge branch 'master' into auto-update-for-materials-in-config-repos
chadlwilson Jun 12, 2023
b931f50
test
chadlwilson Jun 9, 2023
bff7f35
Make auto update mutation clearer
chadlwilson Jun 12, 2023
71eb7e9
Merge remote-tracking branch 'upstream/master' into auto-update-for-m…
chadlwilson Jul 15, 2023
70b749d
Merge remote-tracking branch 'upstream/master' into auto-update-for-m…
chadlwilson Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .../java/com/thoughtworks/go/plugin/configrepo/contract/material/CRPluggableScmMaterial.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ public class CRPluggableScmMaterial extends CRMaterial implements SourceCodeMate
@SerializedName("filter")
@Expose
private CRFilter filter;
@SerializedName("auto_update")
@Expose
private boolean autoUpdate = true;

@SerializedName("plugin_configuration")
@Expose
Expand Down
16 changes: 10 additions & 6 deletions server/src/main/java/com/thoughtworks/go/config/ConfigConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,9 +395,11 @@ private PluggableSCMMaterialConfig toPluggableScmMaterialConfig(CRPluggableScmMa
throw new ConfigConvertionException(
String.format("Failed to find referenced scm '%s'", id));

return new PluggableSCMMaterialConfig(toMaterialName(crPluggableScmMaterial.getName()),
scmConfig, crPluggableScmMaterial.getDestination(),
toFilter(crPluggableScmMaterial.getFilterList()), crPluggableScmMaterial.isWhitelist());
PluggableSCMMaterialConfig materialConfig = new PluggableSCMMaterialConfig(toMaterialName(crPluggableScmMaterial.getName()),
scmConfig, crPluggableScmMaterial.getDestination(),
toFilter(crPluggableScmMaterial.getFilterList()), crPluggableScmMaterial.isWhitelist());
materialConfig.setAutoUpdate(crPluggableScmMaterial.isAutoUpdate());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually setting the value underneath on the SCM which is already passed into the constructor via the scmConfig. For clarity, it appears that if anywhere, this should be set earlier directly on the de-duplicated SCM to be less confusing.

But it may however highlight a problem with the scope of auto_update and how it works when multiple pipelines use the same logical material but declare different auto_update values.

We might need to understand what the consequences are for this for determinism of behaviour when creating and merging these - at least to ensure it behaves the same way as the non-pluggable SCMs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you mean that the code between line 370 and 392 searches for an duplicated scmConfig?
To avoid that we find a scmConfig that we then modify we should set this flag on the scmConfig before doing the search.

So around line 376 add this

            scmConfig.setAutoUpdate(crPluggableScmMaterial.isAutoUpdate());

and revert the change on lines 399-403

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, possibly directly setting the value on the scmConfig prior to constructing is clearer that you might be modifying an existing (shared) config, since the property belongs to the overall config rather than the pluggable config.

But I'll still need to check any implications. In general the way this stuff is modelled in GoCD is a bit problematic due to the de-duplication of SCMs. It's not possible to, say, have one pipeline with a material being manual and another pipeline with the same scm (url, branch) that is auto updating (polling). This causes indeterminism by design, and confusion to users. It's even more confusing for users/admins if there are a mixture of config repo and UI-created pipelines.

The workaround for this nornally is for users to use blanket **/* denylists in individual pipelines' consumption of a material to vary triggering but that won't stop the polling (where people have a preference for webhooks and want to reduce load on their SCM server) - only stop the triggering of certain pipelines.

So I'll want to verify that the way this change behaves is at least similar to git, svn and other SCMs in its determinism/indeterminism.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the method setCommonScmMaterialMembers (line 480) the autoUpdate flag is set for material of UI-created pipelines. That method is called in toScmMaterialConfig for each type of SCM. There is no handling of SCM in that method, maybe it's handled in a different way.

To verify that it doesn't mess up with the other CSMs we could write a test that...

  1. have a list of SCMs contains the same material, but with autoUpdate=true
  2. call the toPluggableScmMaterialConfig method with autoUpdate=false
  3. check that there is a new scmConfig created and not a reused one.

Let me know if there is anything I can give any help or if you want to have a look on your own into this area. It's a lot of details, but I guess you knows the much better than me. :-)

About the workaround. My goal is to reduce the load on the git server as we have a lot of pipelines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this fix overall is definitely needed, reasonable and should be able to be made work. I'll try and get some time to play "in anger", and can make the minor tweaks.

Perhaps for the non-plugin ones the de-duplication happens at runtime somehow, or I am misinterpreting the "scope" of the de-duplication that is happening here.

Do you mind sharing which SCM plugin you are using/interested in here? Otherwise I'll probably validate with https://github.com/TWChennai/gocd-git-path-material-plugin since I understand it in depth.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using the gocd-yaml-config-plugin and also tested the corresponding json version just to verify that the problem wasn't with the file syntax.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I am asking which SCM plugin you are using, i.e which type of custom material you are trying to configure as auto_update: false.

return materialConfig;
}

private SCMs getSCMs() {
Expand Down Expand Up @@ -972,9 +974,11 @@ private CRPluggableScmMaterial pluggableScmMaterialConfigToCRPluggableScmMateria
throw new ConfigConvertionException(
String.format("Failed to find referenced scm '%s'", id));

return new CRPluggableScmMaterial(pluggableScmMaterialConfig.getName().toString(),
id, pluggableScmMaterialConfig.getFolder(),
pluggableScmMaterialConfig.filter().ignoredFileNames(), pluggableScmMaterialConfig.isInvertFilter());
CRPluggableScmMaterial scmMaterial = new CRPluggableScmMaterial(pluggableScmMaterialConfig.getName().toString(),
id, pluggableScmMaterialConfig.getFolder(),
pluggableScmMaterialConfig.filter().ignoredFileNames(), pluggableScmMaterialConfig.isInvertFilter());
scmMaterial.setAutoUpdate(pluggableScmMaterialConfig.isAutoUpdate());
return scmMaterial;
}

private CRPackageMaterial packageMaterialToCRPackageMaterial(PackageMaterialConfig packageMaterialConfig) {
Expand Down