[JENKINS-61200] in case of authentication issue downloading avatar, refresh credentials and make cache time configurable #637
Conversation
…efresh credentials and make cache time configurable Signed-off-by: Olivier Lamy <olamy@apache.org>
olamy
force-pushed
the
JENKINS-61200-refresh-credentials
branch
from
7af98de
to
89655e6
Compare
|
This pull request fixes 1 alert when merging 89655e6 into f8a21d6 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging cd51fc2 into 863acff - view on LGTM.com fixed alerts:
|
Signed-off-by: Olivier Lamy <olamy@apache.org>
|
This pull request fixes 1 alert when merging 7d11b30 into 863acff - view on LGTM.com fixed alerts:
|
| return CredentialsMatchers.firstOrNull( | ||
| CredentialsProvider.lookupCredentials( | ||
| credentials.getClass(), | ||
| (Item) null, // context |
There was a problem hiding this comment.
That would only found root global credentials and not folder credentials. If the credentials are defined in a folder or in the Multibranch item, this is unlikely to work ?
Don't we have access to the SCMOwner from here ?
There was a problem hiding this comment.
not when this piece of code wants to retrieve the avatar https://github.com/jenkinsci/branch-api-plugin/blob/7332a9019a0109975bfa537fa00a6f98e74e3721/src/main/java/jenkins/branch/MetadataActionFolderIcon.java#L84
There was a problem hiding this comment.
yup agree it's a possible problem..
but not sure how to fix it without changing the branch-api-plugin as well and a lot of refactoring here as well.
could be noted as a limitation.
There was a problem hiding this comment.
I have a better solution but this need changes in scm-api and branch-api as well.
not sure we want this in the same PR
There was a problem hiding this comment.
would this be a regression or edge casewith the new implementation?
There was a problem hiding this comment.
this PR fix issue with the current implementation at least for system credentials.
fixing it for folder as well will need more work, I will start the work for that.
But at least we could merge and release this PR as it's already better than the current code ;)
with this the problem will keep happening but only for credentials defined at folder level.
wdyt?
There was a problem hiding this comment.
yeah and if I understand correctly, if folder credentials are used, the lookup will not find it and no fetch will be done. So worst case scenario, we don't have the avatar anymore, but no fetch, so no account locked down in bitbucket.
Good enough IMO.
Signed-off-by: Olivier Lamy olamy@apache.org
Your checklist for this pull request