Enriches Azure autolinks on commits: issues and pull requests

[sergeibbb]

Description

#3977

Issues and PRs on a commit

An open issue

A closed issue

An open PR

A closed PR

Checklist

• I have followed the guidelines in the Contributing document
• My changes follow the coding style of this project
• My changes build without any errors or warnings
• My changes have been formatted and linted
• My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
• My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
• My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

[sergeibbb]

@axosoft-ramint

Enriched autolinks work. I'm switching to Home. Do you think we can merge what's done without the rest of #3977?
Here are my current changes #3996

What I'm not sure about is whether it's OK that keep saved issue-status -> category data in the API class or there is a better place for it. Move it to the integration?

[sergeibbb]

@sergeibbb !
Here with this new ID converting function we'll need to make sure that other types of enriched autolinks are not broken. What do we have? Jira, GitHub Cloud and Ent, GitLab Cloud and Ent. Anything else to check?

[axosoft-ramint]

In some cases like in Jira, we pass the entire integration in as a provider reference, so remoteProviderIdToIntegrationId will fail here. I think in the short term here, we can first try treating link.provider.id as an integration id and try to get the integration from that, and put that in a try/catch so if it fails, then we can try the solution here (try treating it as a remote provider id and convert first before getting the integration).

But in the long term, we need to distinguish, on pull requests, issues, and autolinks, the difference between a remote provider reference and an integration reference. They should be treated separately and put into different properties. If we cannot accomplish that here, we should treat it as a follow-up task.

[axosoft-ramint]

My short term idea would be something like:

Suggested change

	const integrationId = link.provider ? remoteProviderIdToIntegrationId(link.provider.id) : undefined;
	const integrationId: IntegrationId | undefined = link.provider instanceof Integration ? link.provider.id : isRemoteProviderReference(link.provider) ? remoteProviderIdToIntegrationId(link.provider.id) : undefined;
	let linkIntegration = integrationId != null ? await this.container.integrations.get(integrationId) : undefined;

But in long term, we should make sure we never put an Integration into provider property, reserve that for remote providers, and instead have a separate integration property or something that is an integration reference.

[sergeibbb]

updated

[axosoft-ramint]

Will likely break autolinks for several integrations including Jira, but not really your fault here - we just did a very poor job putting both integrations and remote providers into the same provider property just because they both match the very loose providerreference interface.

We should improve on that. I offer a short-term idea and long-term suggestion below, up to you which you think you can get to for this release.

[axosoft-ramint]

In some cases like in Jira, we pass the entire integration in as a provider reference, so remoteProviderIdToIntegrationId will fail here. I think in the short term here, we can first try treating link.provider.id as an integration id and try to get the integration from that, and put that in a try/catch so if it fails, then we can try the solution here (try treating it as a remote provider id and convert first before getting the integration).

But in the long term, we need to distinguish, on pull requests, issues, and autolinks, the difference between a remote provider reference and an integration reference. They should be treated separately and put into different properties. If we cannot accomplish that here, we should treat it as a follow-up task.

[axosoft-ramint]

We should avoid setting the input typing to unknown here. I know we messed up by treating integrations as remote provider references in several places, but it would be better to work toward addressing that directly rather than loosening the typing here.

[sergeibbb]

@axosoft-ramint Allow it to be astring?
If I require it to be RemoteProviderId it will cause type casting in autolinks.ts where we don't really know an actual type.

[sergeibbb]

updated

[axosoft-ramint]

My short term idea would be something like:

Suggested change

	const integrationId = link.provider ? remoteProviderIdToIntegrationId(link.provider.id) : undefined;
	const integrationId: IntegrationId | undefined = link.provider instanceof Integration ? link.provider.id : isRemoteProviderReference(link.provider) ? remoteProviderIdToIntegrationId(link.provider.id) : undefined;
	let linkIntegration = integrationId != null ? await this.container.integrations.get(integrationId) : undefined;

But in long term, we should make sure we never put an Integration into provider property, reserve that for remote providers, and instead have a separate integration property or something that is an integration reference.