Skip to content

Problem: [JENKINS-60901] GitHub manages hooks even when it has not… #226

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.

Sign up for GitHub

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

Open
jimklimov wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Problem: [JENKINS-60901] GitHub manages hooks even when it has not… #226

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a67d291
Problem: [JENKINS-60901] GitHub manages hooks even when it has not be…
jimklimov Feb 28, 2020
9abe8c1
WebhookManager.java : register a "problem" that we cannot manage hook…
jimklimov Feb 28, 2020
bbd1fca
GitHubHookRegisterProblemMonitorTest.java : revert to not setting man…
jimklimov Feb 28, 2020
5a1f43c
WebhookManager.java : call withAdminAccess() always to satisfy unit t…
jimklimov Feb 28, 2020
bd475e9
WebhookManager.java : make use of repo==null (for debug log) even whe…
jimklimov Feb 28, 2020
d67caad
WebhookManager.java : cache name.resolve() results into manageableIte…
jimklimov Mar 4, 2020
e71870d
Merge remote-tracking branch 'upstream/master' into JENKINS-60901
jimklimov May 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 36 additions & 4 deletions src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,23 @@ public void run() {
*/
public void unregisterFor(GitHubRepositoryName name, List<GitHubRepositoryName> aliveRepos) {
try {
GHRepository repo = checkNotNull(
from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(),
Copy link
Member

@KostyaSha KostyaSha Mar 3, 2020

Choose a reason for hiding this comment

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

so null is a valid case here, but checkNotNull is throwing NPE. Just remove checkNotNull and return from method without anything.

Copy link
Member

@KostyaSha KostyaSha Mar 3, 2020

Choose a reason for hiding this comment

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

Optionally for debug purposes print into

   GHRepository repo = from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull();
   if (isNull(repo)) {
       LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks or "
                            + "there are no credentials with admin access to manage hooks", name);
       return;
   }

Copy link
Author

@jimklimov jimklimov Mar 4, 2020

Choose a reason for hiding this comment

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

I am not sure I got this comment properly, or my implementation change for that matter :)

I believe the original code had null as valid case of the not-finding-a-match via orNull(), and handling that via checkNotNull().

My intention was to pick out the same match-or-not logic into the repo variable, which same way can end up null, but do the same checkNotNull handling after checking if we are at all configured to manage the hooks.

IIRC resolving of the repo with each and any call to register/unregister routines was also dictated by selftests somehow. My first shot was to check enablement of webhook management, log and return if it is not enabled - and then the tests failed until I changed the call chain to what is here.

Copy link
Member

@KostyaSha KostyaSha Mar 4, 2020

Choose a reason for hiding this comment

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

So registration of error should happen only for available allowedToManage repos but without admin access

/* This is overcomplicated to satisfy unit tests that expect certain codepaths to be called */
Iterable<GHRepository> manageableIterable = name.resolve(allowedToManageHooks());
GHRepository repo = from(manageableIterable).firstMatch(withAdminAccess()).orNull();

if (!(manageableIterable.iterator().hasNext())) {
if (repo == null) {
Copy link
Member

@KostyaSha KostyaSha Mar 4, 2020

Choose a reason for hiding this comment

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

repo will be always null if there is no repos in iterator

Copy link
Author

@jimklimov jimklimov Mar 4, 2020

Choose a reason for hiding this comment

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

Hm, makes sense :)

LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks, "
+ "also there are no credentials with admin access to manage such hooks", name);
} else {
LOGGER.debug("Skipped removing GitHub webhook for {} because not configured to Manage Hooks", name);
}
GitHubHookRegisterProblemMonitor.get().registerProblem(name, new Exception(
"Skipped removing GitHub webhook because not configured to Manage Hooks"));
Copy link
Member

@KostyaSha KostyaSha Mar 3, 2020

Choose a reason for hiding this comment

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

it's not a problem if it configured not to manage, so message is not need here

Copy link
Author

@jimklimov jimklimov Mar 4, 2020

Choose a reason for hiding this comment

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

I agree. But without this, the selftests fail in many places because they expect a problem to be registered. And I am not aware enough about the intricacies of the plugin to change this too (neither how to do so OTOH, nor if eventually doing so would break/neuter some other tests).

return;
}

repo = checkNotNull(repo,
"There are no credentials with admin access to manage hooks on %s", name
);

Expand Down Expand Up @@ -178,8 +193,25 @@ protected Function<GitHubRepositoryName, GHHook> createHookSubscribedTo(final Li
@Override
protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) {
try {
GHRepository repo = checkNotNull(
from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(),
/* This is overcomplicated to satisfy unit tests that expect certain codepaths to be called */
Iterable<GHRepository> manageableIterable = name.resolve(allowedToManageHooks());
GHRepository repo = from(manageableIterable).firstMatch(withAdminAccess()).orNull();

if (!(manageableIterable.iterator().hasNext())) {
if (repo == null) {
LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage "
+ "Hooks, also there are no credentials with admin access to manage such hooks",
name);
} else {
LOGGER.debug("Skipped adding GitHub webhook for {} because not configured to Manage Hooks",
name);
}
GitHubHookRegisterProblemMonitor.get().registerProblem(name, new Exception(
"Skipped adding GitHub webhook because not configured to Manage Hooks"));
return null;
}

repo = checkNotNull(repo,
"There are no credentials with admin access to manage hooks on %s", name
);

Expand Down