Toggling behaviour based on the CI variable is a misfeature/regression that I cannot turn off

[aiven-amartin]

#71 Introduced the requirement that the env variable CI be set to true for metrics to be collected. This is a non-buildkite namespace variable that I do not want to blindly introduce into all of my calling contexts. Having a valid BUILDKITE_ANALYTICS_TOKEN variable should be sufficient to guard whether the user intends for metrics to be exported using that credential. Ideally 71 would be reverted and/or allow users without it set to behave as before, and only respect explicit CI=false. Even then, non-prefixed variables really don't play nicely ever, and especially not in a generic CI/build toolchain.

[malclocke]

Hi @aiven-amartin 👋 Thanks for taking the time to raise this issue.

We expected the change in #71 to be low impact, because most CI platforms (Buildkite, Github actions, Gitlab, CircleCI, etc) set CI=true in their build environment. It has become a bit of an unofficial industry standard and is used as a signal by a lot of tooling, including pytest.

That said, we do appreciate the presence of BUILDKITE_ANALYTICS_TOKEN is a reasonable guard to control behaviour, and we don't need to rely on an additional check for the CI variable. So we will update the test collector to no longer rely on the CI env var in the next release and link the change to this issue.

[aiven-amartin]

Thank you! I understand that CI is a widespread variable, but that is part of the problem in our case. It's meaning can be very different in different contexts. We isolate our build environments in built container images, and only whitelist the variables we forward to our scripts, and we try to limit how much behaviour changes between production/ci/local execution. Flags like CI run counter to a lot of this.

[gqmelo]

@gchan thank you for handling #84 so quickly. But unfortunately this issue (#74) is preventing us from updating to the newly released 1.3.0 version to benefit from the fix.
Any chance that this can also be addressed as proposed in #74 (comment) and included in a new release?

[aiven-amartin]

Looking back at this, it seems like I misread #71 , I thought it pulled CI from os.environ directly, but it actually doesn't, it populates it if a variety of magic values are set in https://github.com/buildkite/test-collector-python/blob/main/src/buildkite_test_collector/collector/run_env.py. This is confusing imo but is fine for us.

[gchan]

@gqmelo , #89 should address your issue by removing the CI env var requirement. I'll let you know when it gets merged and a new version of the collector is released.

[aiven-amartin]

@gchan Thanks for the update! I hope you saw my comment earlier though, my bug report was actually incorrect, the current behaviour is not a blocker for us, I didn't realize it was not reading the CI env var, but instead synthesizing from the BUILDKITE_* ones.

[gchan]

@gqmelo we've addressed this issue and released a new version (v1.3.1), the CI env var is now completely ignored and has no influence over test collection.