Skip to content

Conversation

@Dohbedoh
Copy link

@Dohbedoh Dohbedoh commented Dec 18, 2023

JENKINS-63355 Prevent NPEs in cases where the library name is not provided (if a library without name is accessible - in the context of a pipeline - then the library step would fail with a NPE, regardless of the library it tries to find)

Testing done

  • Start a controller
  • Add a Global Pipeline Library pointing to a repo with a hello world step.
  • Create a Folder, use defaults
  • Create a hello world pipeline that uses the above library via the library step. e.g. library "test-steps"; helloWorld()
  • Run the pipeline to confirm that it succeeds
  • Configure Folder
    • Find "Pipeline Libraries" section and click "Add"
    • Ignore the error about the library name and click "Save"
  • Run the pipeline again
  • Make sure that the pipeline succeeds
### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [x] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

@Dohbedoh Dohbedoh requested a review from a team as a code owner December 18, 2023 13:04
@jglick jglick added the bug Something isn't working label Dec 18, 2023
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens if a library with a null name was saved before this change? @LocalData

for (LibraryResolver resolver : ExtensionList.lookup(LibraryResolver.class)) {
for (LibraryConfiguration config : resolver.fromConfiguration(Stapler.getCurrentRequest())) {
if (config.getName().equals(name)) {
if (name.equals(config.getName())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be reverted

for (LibraryResolver resolver : ExtensionList.lookup(LibraryResolver.class)) {
for (LibraryConfiguration cfg : resolver.forJob(run.getParent(), Collections.singletonMap(name, version))) {
if (cfg.getName().equals(name)) {
if (name.equals(cfg.getName())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@Dohbedoh
Copy link
Author

So what happens if a library with a null name was saved before this change?

Actually still fails. Need to rework this. We need to skip those libraries. But probably also tell the user about it.

@jglick jglick marked this pull request as draft May 14, 2024 12:50
@Dohbedoh
Copy link
Author

The IAE in the constructor would interrupt the resolution in:

If only we could pass a TaskListener to the LibraryResolver#forJob, we would be able to skip null named library in the for loop during resolution while logging through the task listener (in the build log). But touching LibraryResolver would require to updated some dependent plugin that implements this extension point. Otherwise, we could log a warning through the class logger (Jenkins log)...
Another solution is to not throw an IAE in the constructor, let LibraryResolvers find those libraries, and handle the skip in the callers (LibraryAdder / LibraryStep).

WDYT ?

@jglick
Copy link
Member

jglick commented May 27, 2025

we could log a warning through the class logger (Jenkins log)

Seems enough?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants