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

GitHub Discussions Search Collator #1744

Open
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

minkimcello
Copy link

@minkimcello minkimcello commented Oct 23, 2024

Hey, I just made a Pull Request!

The GitHub Discussions search collator fetches and indexes all discussions, including their comments and replies, for any specified repository, as long as the Backstage instance is configured with the appropriate GitHub integrations and has the necessary read access to the repository in question.

Screenshot 2024-10-23 at 3 22 33 PM

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@backstage-goalie
Copy link
Contributor

Changed Packages

Package Name Package Path Changeset Bump Current Version
app workspaces/github-discussions/packages/app none v0.0.0
backend workspaces/github-discussions/packages/backend none v0.0.0
@backstage-community/plugin-github-discussions-common workspaces/github-discussions/plugins/github-discussions-common none v0.1.0
@backstage-community/plugin-github-discussions workspaces/github-discussions/plugins/github-discussions none v0.1.0
@backstage-community/plugin-search-backend-module-github-discussions workspaces/github-discussions/plugins/search-backend-module-github-discussions none v0.1.0

minkimcello and others added 28 commits October 31, 2024 08:59
Signed-off-by: Min Kim <[email protected]>
Signed-off-by: Min Kim <[email protected]>
Co-authored-by: Taras Mankovski <[email protected]>
Signed-off-by: Min Kim <[email protected]>
Co-authored-by: Taras Mankovski <[email protected]>
Signed-off-by: Min Kim <[email protected]>
Co-authored-by: Taras Mankovski <[email protected]>
Signed-off-by: Min Kim <[email protected]>
Co-authored-by: Taras Mankovski <[email protected]>
Signed-off-by: Min Kim <[email protected]>
Signed-off-by: Min Kim <[email protected]>
@minkimcello minkimcello marked this pull request as ready for review November 12, 2024 00:31
Copy link
Member

@vinzscam vinzscam left a comment

Choose a reason for hiding this comment

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

great addition!

Comment on lines +166 to +173
assert(url !== null, `Not parsable as a Github URL`);
const { name: repo, owner: org } = url;
assert(org !== null, `Discussions url is missing organization name`);
assert(repo !== null, `Discussion url is missing repository`);

const integration = this.githubIntegration.byUrl(url.href);

assert(integration, `Could not retrieve a Github integration for ${url}`);
Copy link
Member

Choose a reason for hiding this comment

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

can we do all this checks in fromConfig instead? Is there any specific reason why you are using this assert-ts library instead of just throwing plain errors?

Comment on lines +194 to +225
const task = run(function* injection(): Operation<void> {
const scope = yield* useScope();
const results = createQueue<GithubDiscussionFetcherResult, void>();

yield* spawn(function* (): Operation<void> {
try {
yield* fetchGithubDiscussions({
client,
org,
repo,
discussionsBatchSize,
commentsBatchSize,
repliesBatchSize,
logger,
results,
cache,
timeout,
clearCacheOnSuccess,
});
} catch (e) {
logger.log(e);
logger.error(
`Encountered an error while ingesting GitHub Discussions`,
e,
);
}
results.close();
});

documents = toAsyncIterable(results, scope);

yield* suspend();
Copy link
Member

Choose a reason for hiding this comment

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

this isn't a pattern we use in this repository or in the core Backstage. Is it possible to simplifying this using normal promises or native async iterators? Tbh it looks complex and hard to follow

@@ -0,0 +1,5 @@
compressionLevel: mixed
Copy link
Member

Choose a reason for hiding this comment

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

you can skip this file and the .yarn folder

Signed-off-by: Min Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants