-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add fallback logic for job name and build tag in build data #368
Add fallback logic for job name and build tag in build data #368
Conversation
String jobName = job.getName(); | ||
if (StringUtils.isNotBlank(jobName)) { | ||
return jobName; | ||
} | ||
if (envVars != null) { | ||
String envJobBaseName = envVars.get("JOB_BASE_NAME"); | ||
if (StringUtils.isNotBlank(envJobBaseName)) { | ||
return envJobBaseName; | ||
} | ||
String envJobName = envVars.get("JOB_NAME"); | ||
if (StringUtils.isNotBlank(envJobName)) { | ||
return envJobName; | ||
} | ||
} | ||
return "unknown"; |
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.
For the JOB_BASE_NAME
, wouldn't better to check if the env var is set, and if not, use the getJobName
functions?
String jobName = job.getName(); | |
if (StringUtils.isNotBlank(jobName)) { | |
return jobName; | |
} | |
if (envVars != null) { | |
String envJobBaseName = envVars.get("JOB_BASE_NAME"); | |
if (StringUtils.isNotBlank(envJobBaseName)) { | |
return envJobBaseName; | |
} | |
String envJobName = envVars.get("JOB_NAME"); | |
if (StringUtils.isNotBlank(envJobName)) { | |
return envJobName; | |
} | |
} | |
return "unknown"; | |
if (envVars != null) { | |
String envJobBaseName = envVars.get("JOB_BASE_NAME"); | |
if (StringUtils.isNotBlank(envJobBaseName)) { | |
return envJobBaseName; | |
} | |
} | |
return getJobName(run, envVars); |
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 was afraid to change the logic too much, since I didn't know what the difference was between the API and the environment variables. That is why I added using the latter only as a fallback - to minimise the changes in behaviour.
If we can be sure that the env var never contradicts whatever is reported by the API, then surely we can change it.
What do you think?
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 would say that the API and env var should be aligned, but I don't have enough context of the Jenkins core implementation to put my finger on it 😅.
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.
Judging by the core code it does seem to be the case. Still I think we should err on the side of caution.
I have changed getBaseJobName
to delegate to getJobName
similarly to what you suggested, but still left the API as the higher priority option, so the behaviour is the following:
- Try to get the base name from the API
- Fall back to
JOB_BASE_NAME
- Fall back to
getJobName
Requirements for Contributing to this repository
What does this PR do?
Adds fallback behaviour to the logic that populates job name, job base name and build tag:
JOB_NAME
is used to fill itJOB_BASE_NAME
(orJOB_NAME
if base name is not available) is used to fill itjenkins-${JOB_NAME}-${BUILD_NUMBER}
is used to fill itCI Visibility backend requires these values to be non-empty. If any of them is not provided, corresponding events will be dropped.
Description of the Change
Alternate Designs
Possible Drawbacks
Verification Process
The new logic is covered with unit tests.
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/
label attached. If applicable it should have thebackward-incompatible
label attached.do-not-merge/
label attached.kind/
andseverity/
labels attached at least.