search

concourse / concourse-chart

Helm chart to install Concourse
Apache License 2.0
143 stars 176 forks source link

should global resources be enabled by default? #246

Open iedsapala opened 3 years ago

iedsapala commented 3 years ago

I noticed that global resources are enabled by default when using this chart, but are disabled by default when using the "off-the-shelf" concourse binary. Should this feature be enabled by default when using this chart and if so why?

iedsapala commented 3 years ago

additional: I noticed this flag was introduced in an "enabled" state in commit 26cb45b72d0fc72c7b7270d2b980ff538b7623be but the comment doesn't indicate why.

chenbh commented 3 years ago

It's off by default because it breaks some exotic workflows like EC2 IAM profiles, I'm not sure if it was an intentional decision to enable it for the helm chart. We did notice that the chart had it enabled, but nobody reported any issues regarding it so we kinda just let it be. 🤷

We would like it to be on by default because it gives a decent performance boost to resource checking and at this point the feature itself is stable. But until we have workarounds to unblock the exotic workflows, we have to keep it opt-in, this is better tracked at https://github.com/concourse/concourse/issues/4870

taylorsilva commented 3 years ago

Looks like this was added before we changed tactics to avoid this very confusion. The concourse binary should be the source of truth for default values. This wasn't being enforced in PR review until the last year-ish.

iedsapala commented 3 years ago

Our team got bitten by this particular feature flag differing in the chart defaults vs the concourse defaults.

We're using two different git resources, one for input used as the trigger for the job and one for output used to push a release tagging commit, that are pointed at the same branch with the same config. When we did a put to the out resource it created a new version (decorated with [skip ci]) which should have been ignored by the input resource, but instead it got picked up and caused the job to do a new build. Please see the attached image of the pipeline below.

image

Considering this kind of possible situation, and the general feature flag discrepancy between the chart, should this feature flag be disabled by default?