Skip to content
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

[JENKINS-73172] Reuse credentials object reference through scans to avoid frequent duplicated lookups #787

Merged
merged 16 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -295,23 +295,22 @@ public static StandardCredentials lookupScanCredentials(
@CheckForNull String repoOwner) {
if (Util.fixEmpty(scanCredentialsId) == null) {
return null;
} else {
StandardCredentials c = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentials(
StandardUsernameCredentials.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication()
: ACL.SYSTEM,
githubDomainRequirements(apiUri)),
CredentialsMatchers.allOf(
CredentialsMatchers.withId(scanCredentialsId), githubScanCredentialsMatcher()));
if (c instanceof GitHubAppCredentials && repoOwner != null) {
return ((GitHubAppCredentials) c).withOwner(repoOwner);
} else {
return c;
}
}
StandardCredentials c = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentialsInItem(
StandardUsernameCredentials.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication2()
: ACL.SYSTEM2,
githubDomainRequirements(apiUri)),
CredentialsMatchers.allOf(
CredentialsMatchers.withId(scanCredentialsId), githubScanCredentialsMatcher()));

if (c instanceof GitHubAppCredentials && repoOwner != null) {
c = ((GitHubAppCredentials) c).withOwner(repoOwner);
}
return c;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@
if (repoUrl != null) {
withBrowser(new GithubWeb(repoUrl));
}
withCredentials(credentialsId(), null);
}

/**
Expand Down Expand Up @@ -189,7 +188,9 @@
* {@code null} to detect the the protocol based on the credentialsId. Defaults to HTTP if
* credentials are {@code null}. Enables support for blank SSH credentials.
* @return {@code this} for method chaining.
* @deprecated Use {@link #withCredentials(String)} and {@link #withResolver(RepositoryUriResolver)}
*/
@Deprecated
@NonNull
public GitHubSCMBuilder withCredentials(String credentialsId, RepositoryUriResolver uriResolver) {
if (uriResolver == null) {
Expand All @@ -200,6 +201,12 @@
return withCredentials(credentialsId);
}

@NonNull
public GitHubSCMBuilder withResolver(RepositoryUriResolver uriResolver) {
this.uriResolver = uriResolver;
return this;
}

/**
* Returns a {@link RepositoryUriResolver} according to credentials configuration.
*
Expand All @@ -215,12 +222,12 @@
return HTTPS;
} else {
StandardCredentials credentials = CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentials(
CredentialsProvider.lookupCredentialsInItem(
StandardCredentials.class,
context,
context instanceof Queue.Task
? ((Queue.Task) context).getDefaultAuthentication()
: ACL.SYSTEM,
? ((Queue.Task) context).getDefaultAuthentication2()
: ACL.SYSTEM2,

Check warning on line 230 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMBuilder.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 225-230 are not covered by tests
URIRequirementBuilder.create()
.withHostname(RepositoryUriResolver.hostnameFromApiUri(apiUri))
.build()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@
private transient Boolean buildForkPRHead;

private static final LoadingCache<String, Boolean> privateModeCache = createPrivateModeCache();
/** The cache of the credentials object */
@CheckForNull
private transient volatile StandardCredentials credentials;

/**
* Constructor.
Expand Down Expand Up @@ -252,6 +255,15 @@
}
}

@CheckForNull
rsandell marked this conversation as resolved.
Show resolved Hide resolved
@Restricted(NoExternalUse.class)
private StandardCredentials getCredentials(@CheckForNull Item context, boolean forceRefresh) {
if (credentials == null || forceRefresh) {

Check warning on line 261 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 261 is only partially covered, 3 branches are missing
credentials = Connector.lookupScanCredentials(context, getApiUri(), getCredentialsId(), getRepoOwner());
}
return credentials;
}

/**
* Gets the API endpoint for the GitHub server.
*
Expand Down Expand Up @@ -962,9 +974,7 @@
throw new AbortException("Must specify user or organization");
}

StandardCredentials credentials =
Connector.lookupScanCredentials((Item) observer.getContext(), apiUri, credentialsId, repoOwner);

StandardCredentials credentials = getCredentials(observer.getContext(), true);
// Github client and validation
GitHub github = Connector.connect(apiUri, credentials);
try {
Expand Down Expand Up @@ -1278,9 +1288,7 @@
throw new AbortException("Must specify user or organization");
}

StandardCredentials credentials =
Connector.lookupScanCredentials((Item) observer.getContext(), apiUri, credentialsId, repoOwner);

StandardCredentials credentials = getCredentials(observer.getContext(), false);
// Github client and validation
GitHub github;
try {
Expand Down Expand Up @@ -1544,9 +1552,7 @@
listener.getLogger().printf("Looking up details of %s...%n", getRepoOwner());
List<Action> result = new ArrayList<>();
String apiUri = Util.fixEmptyAndTrim(getApiUri());
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) owner, getApiUri(), credentialsId, repoOwner);
GitHub hub = Connector.connect(getApiUri(), credentials);
GitHub hub = Connector.connect(getApiUri(), getCredentials(owner, true));
Connector.configureLocalRateLimitChecker(listener, hub);
boolean privateMode = !isEnableAvatar() || determinePrivateMode(apiUri);
try {
Expand Down Expand Up @@ -1641,349 +1647,348 @@
public void afterSave(@NonNull SCMNavigatorOwner owner) {
GitHubWebHook.get().registerHookFor(owner);
try {
// FIXME MINOR HACK ALERT

Check warning on line 1650 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java

View check run for this annotation

ci.jenkins.io / Open Tasks Scanner

FIXME

HIGH: MINOR HACK ALERT
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) owner, getApiUri(), credentialsId, repoOwner);
GitHub hub = Connector.connect(getApiUri(), credentials);
GitHub hub = Connector.connect(getApiUri(), getCredentials(owner, true));
try {
GitHubOrgWebHook.register(hub, repoOwner);
} finally {
Connector.release(hub);
}
} catch (IOException e) {
DescriptorImpl.LOGGER.log(Level.WARNING, e.getMessage(), e);
}
}

@Symbol("github")
@Extension
public static class DescriptorImpl extends SCMNavigatorDescriptor implements IconSpec {

private static final Logger LOGGER = Logger.getLogger(DescriptorImpl.class.getName());

@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public static final String defaultIncludes = "*";

@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public static final String defaultExcludes = "";

public static final String SAME = GitHubSCMSource.DescriptorImpl.SAME;

@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public static final boolean defaultBuildOriginBranch = true;

@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public static final boolean defaultBuildOriginBranchWithPR = true;

@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public static final boolean defaultBuildOriginPRMerge = false;

@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public static final boolean defaultBuildOriginPRHead = false;

@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public static final boolean defaultBuildForkPRMerge = false;

@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public static final boolean defaultBuildForkPRHead = false;

/** {@inheritDoc} */
@Override
public String getPronoun() {
return Messages.GitHubSCMNavigator_Pronoun();
}

/** {@inheritDoc} */
@Override
public String getDisplayName() {
return Messages.GitHubSCMNavigator_DisplayName();
}

/** {@inheritDoc} */
@Override
public String getDescription() {
return Messages.GitHubSCMNavigator_Description();
}

/** {@inheritDoc} */
@Override
public String getIconFilePathPattern() {
return "plugin/github-branch-source/images/github-scmnavigator.svg";
}

/** {@inheritDoc} */
@Override
public String getIconClassName() {
return "icon-github-scm-navigator";
}

/** {@inheritDoc} */
@SuppressWarnings("unchecked")
@Override
public SCMNavigator newInstance(String name) {
GitHubSCMNavigator navigator = new GitHubSCMNavigator(name);
navigator.setTraits(getTraitsDefaults());
navigator.setApiUri(GitHubServerConfig.GITHUB_URL);
return navigator;
}

/** {@inheritDoc} */
@NonNull
@Override
protected SCMSourceCategory[] createCategories() {
return new SCMSourceCategory[] {
new UncategorizedSCMSourceCategory(Messages._GitHubSCMNavigator_UncategorizedCategory())
// TODO add support for forks
};
}

/**
* Validates the selected credentials.
*
* @param context the context.
* @param apiUri the end-point.
* @param credentialsId the credentials.
* @return validation results.
* @since 2.2.0
*/
@RequirePOST
@Restricted(NoExternalUse.class) // stapler
public FormValidation doCheckCredentialsId(
@CheckForNull @AncestorInPath Item context,
@QueryParameter String apiUri,
@QueryParameter String credentialsId,
@QueryParameter String repoOwner) {
return Connector.checkScanCredentials(context, apiUri, credentialsId, repoOwner);
}

/**
* Populates the drop-down list of credentials.
*
* @param context the context.
* @param apiUri the end-point.
* @param credentialsId the existing selection;
* @return the drop-down list.
* @since 2.2.0
*/
@Restricted(NoExternalUse.class) // stapler
public ListBoxModel doFillCredentialsIdItems(
@CheckForNull @AncestorInPath Item context,
@QueryParameter String apiUri,
@QueryParameter String credentialsId) {
if (context == null
? !Jenkins.get().hasPermission(Jenkins.MANAGE)
: !context.hasPermission(Item.EXTENDED_READ)) {
return new StandardListBoxModel().includeCurrentValue(credentialsId);
}
return Connector.listScanCredentials(context, apiUri);
}

/**
* Returns the available GitHub endpoint items.
*
* @return the available GitHub endpoint items.
*/
@Restricted(NoExternalUse.class) // stapler
@SuppressWarnings("unused") // stapler
public ListBoxModel doFillApiUriItems() {
return getPossibleApiUriItems();
}

static ListBoxModel getPossibleApiUriItems() {
ListBoxModel result = new ListBoxModel();
result.add("GitHub", "");
for (Endpoint e : GitHubConfiguration.get().getEndpoints()) {
result.add(
e.getName() == null ? e.getApiUri() : e.getName() + " (" + e.getApiUri() + ")", e.getApiUri());
}
return result;
}

/**
* Returns {@code true} if there is more than one GitHub endpoint configured, and consequently
* the UI should provide the ability to select the endpoint.
*
* @return {@code true} if there is more than one GitHub endpoint configured.
*/
@SuppressWarnings("unused") // jelly
public boolean isApiUriSelectable() {
return !GitHubConfiguration.get().getEndpoints().isEmpty();
}

/**
* Returns the {@link SCMTraitDescriptor} instances grouped into categories.
*
* @return the categorized list of {@link SCMTraitDescriptor} instances.
* @since 2.2.0
*/
@SuppressWarnings("unused") // jelly
public List<NamedArrayList<? extends SCMTraitDescriptor<?>>> getTraitsDescriptorLists() {
GitHubSCMSource.DescriptorImpl sourceDescriptor =
Jenkins.get().getDescriptorByType(GitHubSCMSource.DescriptorImpl.class);
List<SCMTraitDescriptor<?>> all = new ArrayList<>();
all.addAll(SCMNavigatorTrait._for(this, GitHubSCMNavigatorContext.class, GitHubSCMSourceBuilder.class));
all.addAll(SCMSourceTrait._for(sourceDescriptor, GitHubSCMSourceContext.class, null));
all.addAll(SCMSourceTrait._for(sourceDescriptor, null, GitHubSCMBuilder.class));
Set<SCMTraitDescriptor<?>> dedup = new HashSet<>();
for (Iterator<SCMTraitDescriptor<?>> iterator = all.iterator(); iterator.hasNext(); ) {
SCMTraitDescriptor<?> d = iterator.next();
if (dedup.contains(d) || d instanceof GitBrowserSCMSourceTrait.DescriptorImpl) {
// remove any we have seen already and ban the browser configuration as it will always be
// github
iterator.remove();
} else {
dedup.add(d);
}
}
List<NamedArrayList<? extends SCMTraitDescriptor<?>>> result = new ArrayList<>();
NamedArrayList.select(
all,
"Repositories",
new NamedArrayList.Predicate<SCMTraitDescriptor<?>>() {
@Override
public boolean test(SCMTraitDescriptor<?> scmTraitDescriptor) {
return scmTraitDescriptor instanceof SCMNavigatorTraitDescriptor;
}
},
true,
result);
NamedArrayList.select(
all,
Messages.GitHubSCMNavigator_withinRepository(),
NamedArrayList.anyOf(
NamedArrayList.withAnnotation(Discovery.class),
NamedArrayList.withAnnotation(Selection.class)),
true,
result);
NamedArrayList.select(all, Messages.GitHubSCMNavigator_general(), null, true, result);
return result;
}

@SuppressWarnings("unused") // jelly
@NonNull
public List<SCMTrait<? extends SCMTrait<?>>> getTraitsDefaults() {
return new ArrayList<>(ExtensionList.lookupSingleton(GitHubSCMSource.DescriptorImpl.class)
.getTraitsDefaults());
}

static {
IconSet.icons.addIcon(new Icon(
"icon-github-scm-navigator icon-sm",
"plugin/github-branch-source/images/svgs/github-scmnavigator.svg",
Icon.ICON_SMALL_STYLE));
IconSet.icons.addIcon(new Icon(
"icon-github-scm-navigator icon-md",
"plugin/github-branch-source/images/svgs/github-scmnavigator.svg",
Icon.ICON_MEDIUM_STYLE));
IconSet.icons.addIcon(new Icon(
"icon-github-scm-navigator icon-lg",
"plugin/github-branch-source/images/svgs/github-scmnavigator.svg",
Icon.ICON_LARGE_STYLE));
IconSet.icons.addIcon(new Icon(
"icon-github-scm-navigator icon-xlg",
"plugin/github-branch-source/images/svgs/github-scmnavigator.svg",
Icon.ICON_XLARGE_STYLE));

IconSet.icons.addIcon(new Icon(
"icon-github-repo icon-sm",
"plugin/github-branch-source/images/svgs/sprite-github.svg#github-repo",
Icon.ICON_SMALL_STYLE,
IconFormat.EXTERNAL_SVG_SPRITE));
IconSet.icons.addIcon(new Icon(
"icon-github-repo icon-md",
"plugin/github-branch-source/images/svgs/sprite-github.svg#github-repo",
Icon.ICON_MEDIUM_STYLE,
IconFormat.EXTERNAL_SVG_SPRITE));
IconSet.icons.addIcon(new Icon(
"icon-github-repo icon-lg",
"plugin/github-branch-source/images/svgs/sprite-github.svg#github-repo",
Icon.ICON_LARGE_STYLE,
IconFormat.EXTERNAL_SVG_SPRITE));
IconSet.icons.addIcon(new Icon(
"icon-github-repo icon-xlg",
"plugin/github-branch-source/images/svgs/sprite-github.svg#github-repo",
Icon.ICON_XLARGE_STYLE,
IconFormat.EXTERNAL_SVG_SPRITE));
}
}

/** A {@link SCMNavigatorRequest.Witness} that counts how many sources have been observed. */
private static class WitnessImpl implements SCMNavigatorRequest.Witness {
/** The count of repositories matches. */
@GuardedBy("this")
private int count;
/** The listener to log to. */
@NonNull
private final TaskListener listener;

/**
* Constructor.
*
* @param listener the listener to log to.
*/
public WitnessImpl(@NonNull TaskListener listener) {
this.listener = listener;
}

/** {@inheritDoc} */
@Override
public void record(@NonNull String name, boolean isMatch) {
if (isMatch) {
listener.getLogger().format("Proposing %s%n", name);
synchronized (this) {
count++;
}
} else {
listener.getLogger().format("Ignoring %s%n", name);
}
}

/**
* Returns the count of repositories matches.
*
* @return the count of repositories matches.
*/
public synchronized int getCount() {
return count;
}
}

/** Our {@link SCMNavigatorRequest.SourceLambda}. */
private class SourceFactory implements SCMNavigatorRequest.SourceLambda {
/** The request. */
private final GitHubSCMNavigatorRequest request;

/**
* Constructor.
*
* @param request the request to decorate {@link SCMSource} instances with.
*/
public SourceFactory(GitHubSCMNavigatorRequest request) {
this.request = request;
}

/** {@inheritDoc} */
@NonNull
@Override
public SCMSource create(@NonNull String name) {
return new GitHubSCMSourceBuilder(getId() + "::" + name, apiUri, credentialsId, repoOwner, name)
.withRequest(request)
.withCredentials(credentials)

Check warning on line 1991 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 1651-1991 are not covered by tests
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static org.apache.commons.lang.StringUtils.removeEnd;
import static org.jenkinsci.plugins.github_branch_source.Connector.isCredentialValid;
import static org.jenkinsci.plugins.github_branch_source.GitHubSCMBuilder.API_V3;
import static org.jenkinsci.plugins.github_branch_source.GitHubSCMBuilder.HTTPS;

import com.cloudbees.jenkins.GitHubWebHook;
import com.cloudbees.plugins.credentials.CredentialsNameProvider;
Expand Down Expand Up @@ -272,6 +273,9 @@
/** The cache of {@link ObjectMetadataAction} instances for each open PR. */
@NonNull
private transient /*effectively final*/ Map<Integer, ContributorMetadataAction> pullRequestContributorCache;
/** The cache of the credentials object */
@CheckForNull
private transient volatile StandardCredentials credentials;

/**
* Used during upgrade from 1.x to 2.2.0+ only.
Expand Down Expand Up @@ -317,6 +321,19 @@
this.traits = new ArrayList<>();
}

/**
* Constructor that passes a looked up credentials object.
*
* @param repoOwner the repository owner.
* @param repository the repository name.
* @param credentials a {@link com.cloudbees.plugins.credentials.common.StandardCredentials}
*/
@Restricted(NoExternalUse.class)
GitHubSCMSource(String repoOwner, String repository, StandardCredentials credentials) {
this(repoOwner, repository, null, false);
this.credentials = credentials;
}

Check warning on line 335 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 333-335 are not covered by tests

/**
* Legacy constructor.
*
Expand Down Expand Up @@ -362,6 +379,15 @@
}
}

@CheckForNull
rsandell marked this conversation as resolved.
Show resolved Hide resolved
@Restricted(NoExternalUse.class)
private StandardCredentials getCredentials(@CheckForNull Item context, boolean forceRefresh) {
if (credentials == null || forceRefresh) {

Check warning on line 385 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 385 is only partially covered, one branch is missing
credentials = Connector.lookupScanCredentials(context, getApiUri(), getCredentialsId(), getRepoOwner());
}
return credentials;
}

@Restricted(NoExternalUse.class)
public boolean isConfiguredByUrl() {
return repositoryUrl != null;
Expand Down Expand Up @@ -598,28 +624,28 @@
/** {@inheritDoc} */
@Override
public String getRemote() {
return GitHubSCMBuilder.uriResolver(getOwner(), apiUri, credentialsId)
.getRepositoryUri(apiUri, repoOwner, repository);
// Only HTTPS is applicable to the source remote with Username / Password credentials
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can be sure it is only username / password credentials?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rsandell Per my understanding, we can only have a HTTPS resolver from GitHubSCMSource. Only StandardUsernamePasswordCredentials can be selected and used to connect to the GitHub API:

I am not sure why it is not made more explicit with the typing.. But I don't see a scenario where it could be different and that makes sense since this SCM Source is using the GitHub Rest API..

The only specific scenario where we would see SSH if with the SSHCheckoutTrait and that is handled by the trait implementation.

Copy link
Member

Choose a reason for hiding this comment

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

It could be an app credential couldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be an app credential couldn't it?

Yes it is, but GitHubAppCredentials implements StandardUsernamePasswordCredentials and is used for REST calls over HTTPS as well as mentioned Alan. So "HTTPS resolver only" here looks good to me.

return HTTPS.getRepositoryUri(apiUri, repoOwner, repository);
}

/** {@inheritDoc} */
@Override
public String getPronoun() {
return Messages.GitHubSCMSource_Pronoun();
}

/**
* Returns a {@link RepositoryUriResolver} according to credentials configuration.
*
* @return a {@link RepositoryUriResolver}
* @deprecated use {@link GitHubSCMBuilder#uriResolver()} or {@link
* GitHubSCMBuilder#uriResolver(Item, String, String)}.
*/
@Deprecated
@Restricted(DoNotUse.class)
@RestrictedSince("2.2.0")
public RepositoryUriResolver getUriResolver() {
return GitHubSCMBuilder.uriResolver(getOwner(), apiUri, credentialsId);
return HTTPS;

Check warning on line 648 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 628-648 are not covered by tests
}

@Restricted(NoExternalUse.class)
Expand Down Expand Up @@ -977,8 +1003,10 @@
@CheckForNull SCMHeadEvent<?> event,
@NonNull final TaskListener listener)
throws IOException, InterruptedException {
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
// In case we are in an Organization Scan - i.e. (observer instanceof SCMHeadObserver.Any) - use the cached
// credentials
// https://github.com/jenkinsci/branch-api-plugin/blob/2.1169.va_f810c56e895/src/main/java/jenkins/branch/MultiBranchProjectFactory.java#L262
StandardCredentials credentials = getCredentials(getOwner(), !(observer instanceof SCMHeadObserver.Any));

Check warning on line 1009 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 1009 is only partially covered, one branch is missing
// Github client and validation
final GitHub github = Connector.connect(apiUri, credentials);
try {
Expand Down Expand Up @@ -1279,515 +1307,504 @@
@Override
protected Set<String> retrieveRevisions(@NonNull TaskListener listener, Item retrieveContext)
throws IOException, InterruptedException {
StandardCredentials credentials =
Connector.lookupScanCredentials(retrieveContext, apiUri, credentialsId, repoOwner);
// Github client and validation
final GitHub github = Connector.connect(apiUri, credentials);
final GitHub github = Connector.connect(apiUri, getCredentials(retrieveContext, false));
try {
Connector.configureLocalRateLimitChecker(listener, github);
Set<String> result = new TreeSet<>();

try {
// Input data validation
Connector.checkConnectionValidity(apiUri, listener, credentials, github);

// Input data validation
if (isBlank(repository)) {
throw new AbortException("No repository selected, skipping");
}

String fullName = repoOwner + "/" + repository;
ghRepository = github.getRepository(fullName);
final GHRepository ghRepository = this.ghRepository;
listener.getLogger()
.format(
"Listing %s%n",
HyperlinkNote.encodeTo(ghRepository.getHtmlUrl().toString(), fullName));
resolvedRepositoryUrl = ghRepository.getHtmlUrl();
GitHubSCMSourceContext context =
new GitHubSCMSourceContext(null, SCMHeadObserver.none()).withTraits(traits);
boolean wantBranches = context.wantBranches();
boolean wantTags = context.wantTags();
boolean wantPRs = context.wantPRs();
boolean wantSinglePRs = context.forkPRStrategies().size() == 1
|| context.originPRStrategies().size() == 1;
boolean wantMultiPRs = context.forkPRStrategies().size() > 1
|| context.originPRStrategies().size() > 1;
Set<ChangeRequestCheckoutStrategy> strategies = new TreeSet<>();
strategies.addAll(context.forkPRStrategies());
strategies.addAll(context.originPRStrategies());
for (GHRef ref : ghRepository.listRefs()) {
String name = ref.getRef();
if (name.startsWith(Constants.R_HEADS) && wantBranches) {
String branchName = name.substring(Constants.R_HEADS.length());
listener.getLogger()
.format(
"%n Found branch %s%n",
HyperlinkNote.encodeTo(
resolvedRepositoryUrl + "/tree/" + branchName, branchName));
result.add(branchName);
continue;
}
if (name.startsWith(R_PULL) && wantPRs) {
int index = name.indexOf('/', R_PULL.length());
if (index != -1) {
String number = name.substring(R_PULL.length(), index);
listener.getLogger()
.format(
"%n Found pull request %s%n",
HyperlinkNote.encodeTo(
resolvedRepositoryUrl + "/pull/" + number, "#" + number));
// we are allowed to return "invalid" names so if the user has configured, say
// origin as single strategy and fork as multiple strategies
// we will return PR-5, PR-5-merge and PR-5-head in the result set
// and leave it up to the call to retrieve to determine exactly
// whether the name is actually valid and resolve the correct SCMHead type
//
// this allows this method to avoid an API call for every PR in order to
// determine if the PR is an origin or a fork PR and allows us to just
// use the single (set) of calls to get all refs
if (wantSinglePRs) {
result.add("PR-" + number);
}
if (wantMultiPRs) {
for (ChangeRequestCheckoutStrategy strategy : strategies) {
result.add("PR-" + number + "-"
+ strategy.name().toLowerCase(Locale.ENGLISH));
}
}
}
continue;
}
if (name.startsWith(Constants.R_TAGS) && wantTags) {
String tagName = name.substring(Constants.R_TAGS.length());
listener.getLogger()
.format(
"%n Found tag %s%n",
HyperlinkNote.encodeTo(resolvedRepositoryUrl + "/tree/" + tagName, tagName));
result.add(tagName);
continue;
}
}
listener.getLogger().format("%nFinished listing %s%n%n", fullName);
} catch (WrappedException e) {
try {
e.unwrap();
} catch (RateLimitExceededException rle) {
throw new AbortException(rle.getMessage());
}
}
return result;
} finally {
Connector.release(github);
}
}

@Override
protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener listener, Item retrieveContext)
throws IOException, InterruptedException {
StandardCredentials credentials =
Connector.lookupScanCredentials(retrieveContext, apiUri, credentialsId, repoOwner);
// Github client and validation
final GitHub github = Connector.connect(apiUri, credentials);
final GitHub github = Connector.connect(apiUri, getCredentials(retrieveContext, false));
try {
Connector.configureLocalRateLimitChecker(listener, github);
// Input data validation
if (isBlank(repository)) {
throw new AbortException("No repository selected, skipping");
}

String fullName = repoOwner + "/" + repository;
ghRepository = github.getRepository(fullName);
final GHRepository ghRepository = this.ghRepository;
listener.getLogger()
.format(
"Examining %s%n",
HyperlinkNote.encodeTo(ghRepository.getHtmlUrl().toString(), fullName));
GitHubSCMSourceContext context =
new GitHubSCMSourceContext(null, SCMHeadObserver.none()).withTraits(traits);
Matcher prMatcher = Pattern.compile("^PR-(\\d+)(?:-(.*))?$").matcher(headName);
if (prMatcher.matches()) {
// it's a looking very much like a PR
int number = Integer.parseInt(prMatcher.group(1));
listener.getLogger().format("Attempting to resolve %s as pull request %d%n", headName, number);
try {
GHPullRequest pr = ghRepository.getPullRequest(number);
if (pr != null) {
boolean fork =
!ghRepository.getOwner().equals(pr.getHead().getUser());
Set<ChangeRequestCheckoutStrategy> strategies;
if (context.wantPRs()) {
strategies = fork ? context.forkPRStrategies() : context.originPRStrategies();
} else {
// if not configured, we go with merge
strategies = EnumSet.of(ChangeRequestCheckoutStrategy.MERGE);
}
ChangeRequestCheckoutStrategy strategy;
if (prMatcher.group(2) == null) {
if (strategies.size() == 1) {
strategy = strategies.iterator().next();
} else {
// invalid name
listener.getLogger()
.format(
"Resolved %s as pull request %d but indeterminate checkout strategy, "
+ "please try %s or %s%n",
headName,
number,
headName + "-" + ChangeRequestCheckoutStrategy.HEAD.name(),
headName + "-" + ChangeRequestCheckoutStrategy.MERGE.name());
return null;
}
} else {
strategy = null;
for (ChangeRequestCheckoutStrategy s : strategies) {
if (s.name().toLowerCase(Locale.ENGLISH).equals(prMatcher.group(2))) {
strategy = s;
break;
}
}
if (strategy == null) {
// invalid name;
listener.getLogger()
.format(
"Resolved %s as pull request %d but unknown checkout strategy %s, "
+ "please try %s or %s%n",
headName,
number,
prMatcher.group(2),
headName + "-" + ChangeRequestCheckoutStrategy.HEAD.name(),
headName + "-" + ChangeRequestCheckoutStrategy.MERGE.name());
return null;
}
}
PullRequestSCMHead head =
new PullRequestSCMHead(pr, headName, strategy == ChangeRequestCheckoutStrategy.MERGE);
if (head.isMerge()) {
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
}
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, head, listener, ghRepository);

switch (strategy) {
case MERGE:
try {
prRev.validateMergeHash();
} catch (AbortException e) {
listener.getLogger()
.format(
"Resolved %s as pull request %d: %s.%n%n",
headName, number, e.getMessage());
return null;
}
listener.getLogger()
.format(
"Resolved %s as pull request %d at revision %s merged onto %s as %s%n",
headName,
number,
prRev.getPullHash(),
prRev.getBaseHash(),
prRev.getMergeHash());
break;
default:
listener.getLogger()
.format(
"Resolved %s as pull request %d at revision %s%n",
headName, number, prRev.getPullHash());
break;
}
return prRev;
} else {
listener.getLogger().format("Could not resolve %s as pull request %d%n", headName, number);
}
} catch (FileNotFoundException e) {
// maybe some ****er created a branch or a tag called PR-_
listener.getLogger().format("Could not resolve %s as pull request %d%n", headName, number);
}
}
try {
listener.getLogger().format("Attempting to resolve %s as a branch%n", headName);
GHBranch branch = ghRepository.getBranch(headName);
if (branch != null) {
listener.getLogger()
.format(
"Resolved %s as branch %s at revision %s%n",
headName, branch.getName(), branch.getSHA1());
return new SCMRevisionImpl(new BranchSCMHead(headName), branch.getSHA1());
}
} catch (FileNotFoundException e) {
// maybe it's a tag
}
try {
listener.getLogger().format("Attempting to resolve %s as a tag%n", headName);
GHRef tag = ghRepository.getRef("tags/" + headName);
if (tag != null) {
long tagDate = 0L;
String tagSha = tag.getObject().getSha();
if ("tag".equalsIgnoreCase(tag.getObject().getType())) {
// annotated tag object
try {
GHTagObject tagObject = ghRepository.getTagObject(tagSha);
tagDate = tagObject.getTagger().getDate().getTime();
} catch (IOException e) {
// ignore, if the tag doesn't exist, the probe will handle that correctly
// we just need enough of a date value to allow for probing
}
} else {
try {
GHCommit commit = ghRepository.getCommit(tagSha);
tagDate = commit.getCommitDate().getTime();
} catch (IOException e) {
// ignore, if the tag doesn't exist, the probe will handle that correctly
// we just need enough of a date value to allow for probing
}
}
listener.getLogger().format("Resolved %s as tag %s at revision %s%n", headName, headName, tagSha);
return new GitTagSCMRevision(new GitHubTagSCMHead(headName, tagDate), tagSha);
}
} catch (FileNotFoundException e) {
// ok it doesn't exist
}
listener.error("Could not resolve %s", headName);

// TODO try and resolve as a revision, but right now we'd need to know what branch the
// revision belonged to
// once GitSCMSource has support for arbitrary refs, we could just use that... but given that
// GitHubSCMBuilder constructs the refspec based on the branch name, without a specific
// "arbitrary ref"
// SCMHead subclass we cannot do anything here
return null;
} finally {
Connector.release(github);
}
}

@NonNull
private Set<String> updateCollaboratorNames(
@NonNull TaskListener listener,
@CheckForNull StandardCredentials credentials,
@NonNull GHRepository ghRepository)
throws IOException {
if (credentials == null && (apiUri == null || GITHUB_URL.equals(apiUri))) {
// anonymous access to GitHub will never get list of collaborators and will
// burn an API call, so no point in even trying
listener.getLogger().println("Anonymous cannot query list of collaborators, assuming none");
return collaboratorNames = Collections.emptySet();
} else {
try {
return collaboratorNames = new HashSet<>(ghRepository.getCollaboratorNames());
} catch (FileNotFoundException e) {
// not permitted
listener.getLogger().println("Not permitted to query list of collaborators, assuming none");
return collaboratorNames = Collections.emptySet();
} catch (HttpException e) {
if (e.getResponseCode() == HttpServletResponse.SC_UNAUTHORIZED
|| e.getResponseCode() == HttpServletResponse.SC_NOT_FOUND) {
listener.getLogger().println("Not permitted to query list of collaborators, assuming none");
return collaboratorNames = Collections.emptySet();
} else {
throw e;
}
}
}
}

private static class WrappedException extends RuntimeException {

public WrappedException(Throwable cause) {
super(cause);
}

public void unwrap() throws IOException, InterruptedException {
Throwable cause = getCause();
if (cause instanceof IOException) {
throw (IOException) cause;
}
if (cause instanceof InterruptedException) {
throw (InterruptedException) cause;
}
if (cause instanceof RuntimeException) {
throw (RuntimeException) cause;
}
throw this;
}
}

@NonNull
@Override
protected SCMProbe createProbe(@NonNull SCMHead head, @CheckForNull final SCMRevision revision) throws IOException {
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
// Github client and validation
GitHub github = Connector.connect(apiUri, credentials);
GitHub github = Connector.connect(apiUri, getCredentials(getOwner(), false));
try {
String fullName = repoOwner + "/" + repository;
final GHRepository repo = github.getRepository(fullName);
return new GitHubSCMProbe(apiUri, credentials, repo, head, revision);
} catch (IOException | RuntimeException | Error e) {
throw e;
} finally {
Connector.release(github);
}
}

@Override
@CheckForNull
protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOException, InterruptedException {
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);

// Github client and validation
GitHub github = Connector.connect(apiUri, credentials);
GitHub github = Connector.connect(apiUri, getCredentials(getOwner(), false));
try {
try {
Connector.checkConnectionValidity(apiUri, listener, credentials, github);
Connector.configureLocalRateLimitChecker(listener, github);
String fullName = repoOwner + "/" + repository;
ghRepository = github.getRepository(fullName);
final GHRepository ghRepository = this.ghRepository;
resolvedRepositoryUrl = ghRepository.getHtmlUrl();
if (head instanceof PullRequestSCMHead) {
PullRequestSCMHead prhead = (PullRequestSCMHead) head;
GHPullRequest pr = ghRepository.getPullRequest(prhead.getNumber());
if (prhead.isMerge()) {
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
}
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, ghRepository);
prRev.validateMergeHash();
return prRev;
} else if (head instanceof GitHubTagSCMHead) {
GitHubTagSCMHead tagHead = (GitHubTagSCMHead) head;
GHRef tag = ghRepository.getRef("tags/" + tagHead.getName());
String sha = tag.getObject().getSha();
if ("tag".equalsIgnoreCase(tag.getObject().getType())) {
// annotated tag object
GHTagObject tagObject = ghRepository.getTagObject(sha);
// we want the sha of the tagged commit not the tag object
sha = tagObject.getObject().getSha();
}
return new GitTagSCMRevision(tagHead, sha);
} else {
return new SCMRevisionImpl(
head,
ghRepository
.getRef("heads/" + head.getName())
.getObject()
.getSha());
}
} catch (RateLimitExceededException rle) {
throw new AbortException(rle.getMessage());
}
} finally {
Connector.release(github);
}
}

private static PullRequestSCMRevision createPullRequestSCMRevision(
GHPullRequest pr, PullRequestSCMHead prhead, TaskListener listener, GHRepository ghRepository)
throws IOException, InterruptedException {
String baseHash = pr.getBase().getSha();
String prHeadHash = pr.getHead().getSha();
String mergeHash = null;

if (prhead.isMerge()) {
if (Boolean.FALSE.equals(pr.getMergeable())) {
mergeHash = PullRequestSCMRevision.NOT_MERGEABLE_HASH;
} else if (Boolean.TRUE.equals(pr.getMergeable())) {
String proposedMergeHash = pr.getMergeCommitSha();
GHCommit commit = null;
try {
commit = ghRepository.getCommit(proposedMergeHash);
} catch (FileNotFoundException e) {
listener.getLogger()
.format(
"Pull request %s : github merge_commit_sha not found (%s). Close and reopen the PR to reset its merge hash.%n",
pr.getNumber(), proposedMergeHash);
} catch (IOException e) {
throw new AbortException(
"Error while retrieving pull request " + pr.getNumber() + " merge hash : " + e.toString());
}

if (commit != null) {
List<String> parents = commit.getParentSHA1s();
// Merge commits always merge against the most recent base commit they can detect.
if (parents.size() != 2) {
listener.getLogger()
.format(
"WARNING: Invalid github merge_commit_sha for pull request %s : merge commit %s with parents - %s.%n",
pr.getNumber(), proposedMergeHash, StringUtils.join(parents, "+"));
} else if (!parents.contains(prHeadHash)) {
// This is maintains the existing behavior from pre-2.5.x when the merge_commit_sha is
// out of sync from the requested prHead
listener.getLogger()
.format(
"WARNING: Invalid github merge_commit_sha for pull request %s : Head commit %s does match merge commit %s with parents - %s.%n",
pr.getNumber(), prHeadHash, proposedMergeHash, StringUtils.join(parents, "+"));
} else {
// We found a merge_commit_sha with 2 parents and one matches the prHeadHash
// Use the other parent hash as the base. This keeps the merge hash in sync with head
// and base.
// It is possible that head or base hash will not exist in their branch by the time we
// build
// This is be true (and cause a failure) regardless of how we determine the commits.
mergeHash = proposedMergeHash;
baseHash = prHeadHash.equals(parents.get(0)) ? parents.get(1) : parents.get(0);
}
}
}

// Merge PR jobs always merge against the most recent base branch commit they can detect.
// For an invalid merge_commit_sha, we need to query for most recent base commit separately
if (mergeHash == null) {
baseHash = ghRepository
.getRef("heads/" + pr.getBase().getRef())
.getObject()
.getSha();
}
}

return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash);
}

private static void ensureDetailedGHPullRequest(
GHPullRequest pr, TaskListener listener, GitHub github, GHRepository ghRepository)
throws IOException, InterruptedException {
final long sleep = 1000;
int retryCountdown = mergeableStatusRetries;

while (pr.getMergeable() == null && retryCountdown > 1) {
listener.getLogger()
.format(
"Waiting for GitHub to create a merge commit for pull request %d. Retrying %d more times...%n",
pr.getNumber(), --retryCountdown);
Thread.sleep(sleep);
}
}

@Override
public SCM build(SCMHead head, SCMRevision revision) {
return new GitHubSCMBuilder(this, head, revision).withTraits(traits).build();
}

@CheckForNull
/*package*/ URL getResolvedRepositoryUrl() {
return resolvedRepositoryUrl;
}

@Deprecated // TODO remove once migration from 1.x is no longer supported
PullRequestSource retrievePullRequestSource(int number) {
// we use a big honking great lock to prevent concurrent requests to github during job loading
Map<Integer, PullRequestSource> pullRequestSourceMap;
synchronized (pullRequestSourceMapLock) {
pullRequestSourceMap = this.pullRequestSourceMap;
if (pullRequestSourceMap == null) {
this.pullRequestSourceMap = pullRequestSourceMap = new HashMap<>();
if (StringUtils.isNotBlank(repository)) {
String fullName = repoOwner + "/" + repository;
LOGGER.log(Level.INFO, "Getting remote pull requests from {0}", fullName);
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
LogTaskListener listener = new LogTaskListener(LOGGER, Level.INFO);
try {
GitHub github = Connector.connect(apiUri, credentials);
GitHub github = Connector.connect(apiUri, getCredentials(getOwner(), false));

Check warning on line 1807 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 1311-1807 are not covered by tests
try {
Connector.configureLocalRateLimitChecker(listener, github);
ghRepository = github.getRepository(fullName);
Expand Down Expand Up @@ -1952,8 +1969,7 @@
result.add(new GitHubRepoMetadataAction());
String repository = this.repository;

StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
StandardCredentials credentials = getCredentials(getOwner(), true);
GitHub hub = Connector.connect(apiUri, credentials);
try {
Connector.checkConnectionValidity(apiUri, listener, credentials, hub);
Expand Down Expand Up @@ -2857,74 +2873,71 @@
.format(
"Connecting to %s to obtain list of collaborators for %s/%s%n",
apiUri, repoOwner, repository);
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
StandardCredentials credentials = getCredentials(getOwner(), false);
// Github client and validation
try {
GitHub github = Connector.connect(apiUri, credentials);
try {
Connector.configureLocalRateLimitChecker(listener, github);

// Input data validation
Connector.checkConnectionValidity(apiUri, listener, credentials, github);
// Input data validation
String credentialsName =
credentials == null ? "anonymous access" : CredentialsNameProvider.name(credentials);
if (credentials != null && !isCredentialValid(github)) {
listener.getLogger()
.format(
"Invalid scan credentials %s to connect to %s, "
+ "assuming no trusted collaborators%n",
credentialsName, apiUri);
collaboratorNames = Collections.singleton(repoOwner);
} else {
if (!github.isAnonymous()) {
listener.getLogger().format("Connecting to %s using %s%n", apiUri, credentialsName);
} else {
listener.getLogger()
.format("Connecting to %s with no credentials, anonymous access%n", apiUri);
}

// Input data validation
if (isBlank(getRepository())) {
collaboratorNames = Collections.singleton(repoOwner);
} else {
String fullName = repoOwner + "/" + repository;
ghRepository = github.getRepository(fullName);
resolvedRepositoryUrl = ghRepository.getHtmlUrl();
return new LazyContributorNames(request, listener, github, ghRepository, credentials);
}
}
return collaboratorNames;
} finally {
Connector.release(github);
}
} catch (IOException | InterruptedException e) {
throw new WrappedException(e);
}
}
}

private class DeferredPermissionsSource extends GitHubPermissionsSource implements Closeable {

private final TaskListener listener;
private GitHub github;
private GHRepository repo;

public DeferredPermissionsSource(TaskListener listener) {
this.listener = listener;
}

@Override
public GHPermissionType fetch(String username) throws IOException, InterruptedException {
if (repo == null) {
listener.getLogger()
.format(
"Connecting to %s to check permissions of obtain list of %s for %s/%s%n",
apiUri, username, repoOwner, repository);
StandardCredentials credentials =
Connector.lookupScanCredentials((Item) getOwner(), apiUri, credentialsId, repoOwner);
github = Connector.connect(apiUri, credentials);
github = Connector.connect(apiUri, getCredentials(getOwner(), false));

Check warning on line 2940 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 2876-2940 are not covered by tests
String fullName = repoOwner + "/" + repository;
repo = github.getRepository(fullName);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package org.jenkinsci.plugins.github_branch_source;

import com.cloudbees.plugins.credentials.common.StandardCredentials;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import jenkins.scm.api.trait.SCMSourceBuilder;
Expand All @@ -45,6 +46,9 @@
/** The repository owner. */
@NonNull
private final String repoOwner;
/** The credentials. */
@CheckForNull
private StandardCredentials credentials;

/**
* Constructor.
Expand Down Expand Up @@ -94,24 +98,35 @@
*/
@CheckForNull
public final String credentialsId() {
return credentialsId;
return credentials == null ? credentialsId : credentials.getId();
}

/**
* The repository owner that the {@link GitHubSCMSource} will be configured to use.
*
* @return the repository owner that the {@link GitHubSCMSource} will be configured to use.
*/
@NonNull
public final String repoOwner() {
return repoOwner;
}

/**
* Pass the credentials object to the {@link GitHubSCMSource}.
* @param credentials the {@link com.cloudbees.plugins.credentials.common.StandardCredentials}
* @return the current builder
*/
@NonNull
SCMSourceBuilder<GitHubSCMSourceBuilder, GitHubSCMSource> withCredentials(StandardCredentials credentials) {
this.credentials = credentials;
return this;
}

/** {@inheritDoc} */
@NonNull
@Override
public GitHubSCMSource build() {
GitHubSCMSource result = new GitHubSCMSource(repoOwner, projectName());
GitHubSCMSource result = new GitHubSCMSource(repoOwner, projectName(), credentials);

Check warning on line 129 in src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceBuilder.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 101-129 are not covered by tests
Copy link
Contributor

@jeromepochat jeromepochat Oct 11, 2024

Choose a reason for hiding this comment

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

This looks to me that credentials and credentialsId could diverge. I think that credentialsId() should return credentials.getId() in case credentials is set. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Would be more consistent to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeromepochat I applied your suggestion.

result.setId(id());
result.setApiUri(apiUri());
result.setCredentialsId(credentialsId());
Expand Down
Loading
Loading