-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
jenkinsfiles: remove unused environment variables #15125
Conversation
test-me-please |
hit #15141 |
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.
Thanks for taking a stab at this! There are some issues with this change, let me know if any of my comments is unclear.
jenkinsfiles/ginkgo-gke.Jenkinsfile
Outdated
@@ -83,6 +71,7 @@ pipeline { | |||
} | |||
if (env.RACE?.trim()) { |
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.
s/env.RACE?.trim()/env.run_with_race_detection?.trim()
, RACE
was set at line 23
@@ -100,6 +88,7 @@ pipeline { | |||
} | |||
if (env.RACE?.trim()) { |
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.
same as gke jenkinsfile
jenkinsfiles/ginkgo-gke.Jenkinsfile
Outdated
@@ -83,6 +71,7 @@ pipeline { | |||
} | |||
if (env.RACE?.trim()) { | |||
env.DOCKER_TAG = env.DOCKER_TAG + "-race" | |||
env.RACE = "true" |
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.
This doesn't do anything, let's get rid of RACE
env var in all jenkinsfiles aside from runtime.
@@ -100,6 +88,7 @@ pipeline { | |||
} | |||
if (env.RACE?.trim()) { | |||
env.DOCKER_TAG = env.DOCKER_TAG + "-race" | |||
env.RACE = "true" |
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.
same as gke jenkinsfile
@@ -28,14 +28,6 @@ pipeline { | |||
returnStdout: true, | |||
script: 'if [ "${run_with_race_detection}" = "" ]; then echo -n ""; else echo -n "1"; fi' | |||
)}""" | |||
LOCKDEBUG="""${sh( |
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.
I think we want these env vars to be set, as they are passed to runtime VM which causes them to be passed to make
targets run inside the vm which build the binaries.
returnStdout: true, | ||
script: 'if [ "${run_with_race_detection}" = "" ]; then echo -n ""; else echo -n "1"; fi' | ||
)}""" | ||
BASE_IMAGE="""${sh( |
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.
ditto
@@ -87,6 +75,7 @@ pipeline { | |||
env.DOCKER_TAG = env.ghprbActualCommit | |||
} else { | |||
env.DOCKER_TAG = env.GIT_COMMIT | |||
env.RACE = "true" |
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.
This is a no-op, we need a second if in this block:
if (env.run_with_race_detection?.trim()) {
env.DOCKER_TAG = env.DOCKER_TAG + "-race"
}
As we are no longer building images in ginkgo, we can remove these environment variables. Signed-off-by: André Martins <andre@cilium.io>
64d88d1
to
9ace2ea
Compare
test-me-please |
merging since this is only modifying jenkinsfiles. |
PR cilium#15125 broke race detector pipelines due to the `RACE` environment variable not being present, when it is required for `SkipRaceDetectorEnabled` to work properly. A quick inspection leads me to believe `LOCKDEBUG` also is required for package `lock`. I am less convinced about `BASE_IMAGE` being required, but I'm adding it back anyway in a sort of spiritual revert until we figure it out. Fixes cilium#15214: the Kafka tests were actually already disabled for race detector pipelines, as per cilium#13757. They were only running the pipelines due to cilium#15125 re-enabling them incidentally. Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
PR #15125 broke race detector pipelines due to the `RACE` environment variable not being present, when it is required for `SkipRaceDetectorEnabled` to work properly. A quick inspection leads me to believe `LOCKDEBUG` also is required for package `lock`. I am less convinced about `BASE_IMAGE` being required, but I'm adding it back anyway in a sort of spiritual revert until we figure it out. Fixes #15214: the Kafka tests were actually already disabled for race detector pipelines, as per #13757. They were only running the pipelines due to #15125 re-enabling them incidentally. Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
[ upstream commit f578ba5 ] PR cilium#15125 broke race detector pipelines due to the `RACE` environment variable not being present, when it is required for `SkipRaceDetectorEnabled` to work properly. A quick inspection leads me to believe `LOCKDEBUG` also is required for package `lock`. I am less convinced about `BASE_IMAGE` being required, but I'm adding it back anyway in a sort of spiritual revert until we figure it out. Fixes cilium#15214: the Kafka tests were actually already disabled for race detector pipelines, as per cilium#13757. They were only running the pipelines due to cilium#15125 re-enabling them incidentally. Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com> Signed-off-by: Glib Smaga <code@gsmaga.com>
[ upstream commit f578ba5 ] PR #15125 broke race detector pipelines due to the `RACE` environment variable not being present, when it is required for `SkipRaceDetectorEnabled` to work properly. A quick inspection leads me to believe `LOCKDEBUG` also is required for package `lock`. I am less convinced about `BASE_IMAGE` being required, but I'm adding it back anyway in a sort of spiritual revert until we figure it out. Fixes #15214: the Kafka tests were actually already disabled for race detector pipelines, as per #13757. They were only running the pipelines due to #15125 re-enabling them incidentally. Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com> Signed-off-by: Glib Smaga <code@gsmaga.com>
As we are no longer building images in ginkgo, we can remove these
environment variables.
Signed-off-by: André Martins andre@cilium.io