-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Report whether k6 runs on CI #4063
Conversation
Co-authored-by: Inanc Gumus <m@inanc.io>
Removing myself from the reviewers as we pair-coded this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach with the CI looks good to me 👍 Left a few non-blocking suggestions.
Quickly searching on GitHub (or I believe there could be other popular packages) seems like there could be a couple of more environment variables that we can look into.
Including such environment variables still sounds pretty safe, but our statistics will be more accurate, and unlikely we return to adjusting this code soon, so it's good to consider them since the beginning.
Considering these two comments, what do you think about something like the following: // isCI is a helper that follows a naive approach to determine if k6 is being
// executed within a CI system. This naive approach consists of checking if
// the "CI" environment variable (among others) is set.
//
// We treat the "CI" environment variable carefully, because it's the one
// used more often, and because we know sometimes it's explicitly set to
// "false" to signal that k6 is not running in a CI environment.
//
// It is not a foolproof method, but it should work for most cases.
func isCI(lookupEnv func(key string) (val string, ok bool)) bool {
if ci, ok := lookupEnv("CI"); ok {
ciBool, err := strconv.ParseBool(ci)
if err == nil {
return ciBool
}
// If we can't parse the "CI" value as a bool, we assume return true
// because we know that at least it's not set to any variant of "false",
// which is the most common use case, and the reasoning we apply below.
return true
}
// List of common environment variables used by different CI systems.
ciEnvVars := []string{
"BUILD_ID", // Jenkins, Cloudbees
"BUILD_NUMBER", // Jenkins, TeamCity
"CI", // Travis CI, CircleCI, Cirrus CI, Gitlab CI, Appveyor, CodeShip, dsari
"CI_APP_ID", // Appflow
"CI_BUILD_ID", // Appflow
"CI_BUILD_NUMBER", // Appflow
"CI_NAME", // Codeship and others
"CONTINUOUS_INTEGRATION", // Travis CI, Cirrus CI
"RUN_ID", // TaskCluster, dsari
}
// Check if any of these variables are set
for _, key := range ciEnvVars {
if _, ok := lookupEnv(key); ok {
return true
}
}
return false
} cc/ @inancgumus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but IMO the current name name in the report is not accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
521f080
7ec8537
to
8664f4e
Compare
What?
It adds a new field into the usage report that tells whether the
CI
environment variable is defined or not when k6 is executed.Why?
Because as part of understanding how users use k6, we want to know how many of total k6 executions happen in a CI system, and although it's probably difficult to have a complete picture, this is the first naive approach to start collecting that data, as the
CI
environment variable is some sort of non-written standard, that most of the CI systems (and other tools) use, and set.Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)
Closes #4038