-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Unifies autolink enrichment keys for integrations #4565
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
Conversation
I'm making this change because Linear is another integration that needs a full issue key rather than just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - one nitpick
|
||
await this.refreshSessionIfExpired(scope); | ||
|
||
const issueOrPR = this.container.cache.getIssueOrPullRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update the cache function's name and parameter as well since this has been updated everywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axosoft-ramint
I'm not sure about that. Integration provider is smart while cache is stupid.
Integration provider is smart enough to understand why it have to pick among two incoming strings (id and key). So, when a developer creates, e.g. Gerrit integration it knows that it's a Gerrit. getLinkedIssueOrPullRequest()
and picks a better value for retrieving the linked object by API.
While cache is just a key-value storage that stores issues or PR: Hey cache, take a key and give me whatever you have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axosoft-ramint
well, now I'm thinking that maybe it's worth to add type (type?: IssueOrPullRequestType
) to the cache key, because it plays role in the retrieving algorithm (in the provider-specific part of the method).
what do you think?
cada70d
to
6a2578c
Compare
in order to avoid referencing the exact integration type outside of integratin class. Updates the autolink enrichment workflow to consistently use a pair of values (id and key) when resolving issues or pull requests across integrations. This removes provider-specific logic for constructing enrichment keys and centralizes the approach, simplifying maintenance and reducing errors when linking to external systems. Renames and refactors relevant methods in integration classes to reflect this unified signature. Enhances consistency and clarity in how autolinked items are identified and retrieved across supported providers.
6a2578c
to
0bdfbc5
Compare
Description
Unifies autolink enrichment keys for integrations in order to avoid referencing the exact integration type outside of integratin class.
Updates the autolink enrichment workflow to consistently use a pair of values (id and key) when resolving issues or pull requests across integrations. This removes provider-specific logic for constructing enrichment keys and centralizes the approach, simplifying maintenance and reducing errors when linking to external systems.
Renames and refactors relevant methods in integration classes to reflect this unified signature. Enhances consistency and clarity in how autolinked items are identified and retrieved across supported providers.
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses