[JENKINS-61290] hard to see why a branch was not inspected - log them

[jimklimov]

• JENKINS issue(s):
  • https://issues.jenkins-ci.org/browse/JENKINS-61290
• Description:
  • When branch is not indexed/scanned because of policy setting, it is not easy to see why it was skipped. This PR adds logging of isExcluded() hits into global jenkins log. Alas, I did not find a way to log it into the indexing run's log itself where it would be more appropriate.
• Documentation changes:
  • No doc change proposed at this time, as this is a small change for trace-logging and no new/changed feature.
• Users/aliases to notify:
  • markewaite

[bitwiseman]

I like this idea, but it might need some tweaking.
I think using LOGGER will result in the messages going to the Jenkins console log, right? Wouldn't we rather they show up in the scan log?

[jimklimov]

Yes, as commented in the top post and in Jira, I unfortunately could not pick a way through all the Java magic to make these messages appear in the indexing/scan log where everyone with access to the job can see them. And which is the first place troubleshooters would look at.

If someone knows how to make that happen - feel free to replace this PR by that better solution :)

[bitwiseman]

@jimklimov
Yeah, it can be difficult, but it is possible. Once you update to use the TaskListener and add some tests, this will be great to get merged.

[jimklimov]

Thanks for the suggestion, the code is at least buildable :) I'd need to schedule a restart of my instance to test this build in practice.

As for tests, the idea is along the lines of setting up a mock github repo with a master branch and at least two other branches, one of which has a PR to master; and somehow invoke the branch-indexing runs with the different strategies (three of them?) and check that the expected amount of branches were discovered, and that the indexing log contains expected lines from isExcluded() where applicable.

I assumed that except the last "indexing log" part, a test setup like this should already exist, but did not find one to extend. Logically BranchDiscoveryTraitTest.java looks like the place, but it seems to only care that expected Java classes for filtering get assigned to the call stack for different setups - and does not actually mock/run the indexing.

[jimklimov]

I have prepared the mock data for some branches and PRs based on sanitized values from our repos that had the original problem (PR made from a branch in the repo and so hid it from indexing with our settings). But I just can't conjure up a way to have these files used by a test - whatever I do, the original mocks are used giving my test experiments completely different data to look at and fail on :(

The "whatever I have before going to sleep" snapshot of that attempt is in https://github.com/jimklimov/github-branch-source-plugin/tree/log-ignored-branches-selftest if you'd care to propose what's wrong in that wild copypasta of things I do not understand :)

[jimklimov]

As for logging - it does land into branch indexing console now, great thanks!

    Checking branch featureimage/modbus-powermeter
      ‘Jenkinsfile’ found
    Met criteria
No changes detected: featureimage/modbus-powermeter (still at dd8e43f53aa9ef08e61908b0b34d60cdc610394a)

    Checking branch featureimage/new-configurator
Ignoring SCMHead{'featureimage/new-configurator'} because current strategy excludes branches that ARE also filed as a pull request
    Checking branch featureimage/systemd-deps
      ‘Jenkinsfile’ found
    Met criteria
No changes detected: featureimage/systemd-deps (still at 580e4c13fd37b2a27e5326536ed00b3444e7d134)

Beside a cosmetic nit of a missing blank line, that makes this message a bit less visible, all looks great :)

[jimklimov]

Newline added, nit resolved :)

    Checking branch featureimage/modbus-powermeter
      ‘Jenkinsfile’ found
    Met criteria
No changes detected: featureimage/modbus-powermeter (still at dd8e43f53aa9ef08e61908b0b34d60cdc610394a)

    Checking branch featureimage/new-nut-configurator
Ignoring SCMHead{'featureimage/new-nut-configurator'} because current strategy excludes branches that ARE also filed as a pull request

    Checking branch featureimage/systemd-deps
      ‘Jenkinsfile’ found
    Met criteria
No changes detected: featureimage/systemd-deps (still at 580e4c13fd37b2a27e5326536ed00b3444e7d134)

[bitwiseman]

Sorry for slow response, I was on on vacation.

Yeah, the testing framework for this plugin really needs work. Can you add the code for your test (even though it fails) and I'll see if I can debug it?

[jimklimov]

Thanks! As mentioned above, the experiments about testing were in https://github.com/jimklimov/github-branch-source-plugin/tree/log-ignored-branches-selftest branch and I did not progress much after that, had a lot of dayjob work to do.

I think the relatively valuable commits there, compared to this PR, are files with (sanitized) saved responses for the GitHub REST API requests that happened for the original problem. The code to get them tested was more of a failed proof of concept and commented illustration of logic of the test I wanted to achieve. That said, I am not in favor of corrupting the buildability and practical usability of this PR by adding broken tests right into it (we run the HPI built from this, it works for us) :)

[MindaugasLaganeckas]

This is a very much needed feature :)

[MarkEWaite]

Looks good to me.

[jglick]

Are all of these new test resource files actually used? I do not see any new (Java) test code. Perhaps these are left over from some commits years earlier, or does some existing test actually fail without them?