feat: Add CICD platform to metrics #2766
Conversation
aahung
force-pushed
the
identify-cicd-telemetry
branch
2 times, most recently
from
c8eefb4 to
c575b58
Compare
aahung
force-pushed
the
identify-cicd-telemetry
branch
from
c575b58 to
54b37c5
Compare
aahung
force-pushed
the
identify-cicd-telemetry
branch
from
54b37c5 to
8c85279
Compare
samcli/lib/telemetry/cicd.py
Outdated
| # https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-env-vars.html | ||
| CICDPlatform.AWSCodeBuild: "CODEBUILD_BUILD_ID", | ||
| # https://www.jetbrains.com/help/teamcity/predefined-build-parameters.html | ||
| CICDPlatform.TeamCity: "CODEBUILD_BUILD_ID", |
There was a problem hiding this comment.
Should this also be CODEBUILD_BUILD_ID?
samcli/lib/telemetry/cicd.py
Outdated
| # without re-calculating because CICD platform won't change once sam-cli starts. | ||
| try: | ||
| _cicd_platform: Optional[CICDPlatform] = next( | ||
| cicd_platform for cicd_platform in CICDPlatform if _is_cicd_platform(cicd_platform, os.environ) |
There was a problem hiding this comment.
Any order consideration in it? For example, if one env var appears in two platform. It will be fall in the first platform.
There was a problem hiding this comment.
And related to this, dict order isn't guaranteed on Python 3.6, so we shouldn't depend on it.
Changed in version 3.7: Dictionary order is guaranteed to be insertion order. This behavior was an implementation detail of CPython from 3.6.
Edit: never mind it's iterating over the enum
There was a problem hiding this comment.
yes, the environment variables are very platform-specific if you look at them. Except for the last one CI, which is placed at the bottom as a "catch-all"
samcli/lib/telemetry/cicd.py
Outdated
| # without re-calculating because CICD platform won't change once sam-cli starts. | ||
| try: | ||
| _cicd_platform: Optional[CICDPlatform] = next( | ||
| cicd_platform for cicd_platform in CICDPlatform if _is_cicd_platform(cicd_platform, os.environ) |
There was a problem hiding this comment.
And related to this, dict order isn't guaranteed on Python 3.6, so we shouldn't depend on it.
Changed in version 3.7: Dictionary order is guaranteed to be insertion order. This behavior was an implementation detail of CPython from 3.6.
Edit: never mind it's iterating over the enum
samcli/lib/telemetry/cicd.py
Outdated
|
|
||
| # detect CI/CD platform. Here we only calculate this value once so platform() can return this value | ||
| # without re-calculating because CICD platform won't change once sam-cli starts. | ||
| try: |
There was a problem hiding this comment.
I'm not sure whether we should have code at the module scope, as this means the code is executed immediately when imported (not the usual expectation). Doesn't seem to be a problem here but could become a footgun later on.
There was a problem hiding this comment.
Great idea, I moved them to Metric's __init__()
4 participants
Which issue(s) does this change fix?
Why is this change necessary?
How does it address the issue?
What side effects does this change have?
Checklist
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.