-
Notifications
You must be signed in to change notification settings - Fork 6
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
Use new subject / schema loader methods / routes when populating the topic and schema views #1052
base: main
Are you sure you want to change the base?
Use new subject / schema loader methods / routes when populating the topic and schema views #1052
Conversation
…ew-controller-new-schema-routes
…ew-controller-new-schema-routes
…ew-controller-new-schema-routes
…ent view controllers.
…ew-controller-new-schema-routes
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.
Copilot reviewed 5 out of 17 changed files in this pull request and generated 1 comment.
Files not reviewed (12)
- src/models/schema.test.ts: Evaluated as low risk
- src/loaders/resourceLoader.test.ts: Evaluated as low risk
- src/quickpicks/schemas.ts: Evaluated as low risk
- src/commands/schemas.ts: Evaluated as low risk
- src/commands/schemaUpload.ts: Evaluated as low risk
- src/viewProviders/schemas.test.ts: Evaluated as low risk
- src/loaders/loaderUtils.test.ts: Evaluated as low risk
- tests/unit/testUtils.ts: Evaluated as low risk
- src/loaders/resourceLoader.ts: Evaluated as low risk
- src/loaders/loaderUtils.ts: Evaluated as low risk
- tests/unit/testResources/schema.ts: Evaluated as low risk
- src/viewProviders/topics.ts: Evaluated as low risk
…ew-controller-new-schema-routes
… over replacement function loadTopicSchemas() which has equivalent tests.
…ew-controller-new-schema-routes
@@ -27,16 +27,15 @@ const logger = new Logger("commands.schemaUpload"); | |||
*/ | |||
|
|||
/** | |||
* Wrapper around {@linkcode uploadSchemaFromFile}, triggered from an inline action on a schema | |||
* subject container in the Schemas view. | |||
* Wrapper around {@linkcode uploadSchemaFromFile}, triggered from an inline actions: |
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.
* Wrapper around {@linkcode uploadSchemaFromFile}, triggered from an inline actions: | |
* Wrapper around {@linkcode uploadSchemaFromFile}, triggered from inline actions: |
* 1. On a Subect treeitem in the Schemas view (passing in a Subject) | ||
* 2. On one of a topic's schema subject group in the Topics view (passing in a ContainerTreeItem<Schema>) |
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.
Is this supposed to be a Subject
and a Schema
? Or a Subject
and a SubjectTreeItem
?
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.
I've read through more, and I think I see where I was confused -- this basically splits it into "was this called from the Topics view (ContainerTreeItem) or Schemas view (Subject)", right?
export class Subject implements IResourceBase, ISearchable { | ||
connectionId!: ConnectionId; | ||
environmentId!: EnvironmentId; | ||
schemaRegistryId!: string; | ||
|
||
name!: string; | ||
id!: string; |
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.
Is there any reason we can't use schemas: Schema[] = []
and an optional type
for when we handle the preloading so we don't have to use ContainerTreeItem<Schema>
in the Topics view? I feel like the Subjectish
split between Subject
and ContainerTreeItem<Schema>
is making this more complicated since we use them in the same way in each view (but the Topics view has the KafkaTopic
parent items). 🤔
@@ -101,11 +101,14 @@ export class TopicViewProvider implements vscode.TreeDataProvider<TopicViewProvi | |||
getTreeItem(element: TopicViewProviderData): vscode.TreeItem | Thenable<vscode.TreeItem> { | |||
let treeItem: vscode.TreeItem; | |||
if (element instanceof KafkaTopic) { | |||
logger.debug("Creating tree item for KafkaTopic", { element }); |
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.
Still need these three debug logs?
Summary of Changes
ResourceLoader.getSubjects()
and.getSchemaSubjectGroup()
instead ofResourceLoader.getSchemas()
. This removes majority of calls to getLIST /schemas
route as spelled byloader.getSchemas...()
.Subject
representing a subject loaded from a particular schema registry. AdjustResourceLoader.getSubjects()
to return array of those instead of bare strings, because of need to know the source registry's particulars when invoking actions like 'evolve schema', 'view latest schema,' etc from the views.Schemas View
ResourceLoader..getSchemaSubjectGroup()
returning aSchema[]
.Topics View
KafkaTopic
. They are built with knowledge of the subjects within the schema registry, and will be expandable if one or more subject correlates by name.ContainerTreeItem<Schema>
as before, populated through getting all of the subjects, then filtering by the topic, then fetching the schema group for the remaining subjects. Because this process requires preemptively fetching all of the schemas for these single-topic-matching-subjects (usually only one or two subjects), we can paint the subjects also with knowledge of the schema type and count of versions.Commands
ContainerTreeItem<Schema>
(from topics view) or aSubject
, so new type union for the argument type for those commands is needed,Subjectish
, representing something from which (subject string, schema registry id, connection id, environment id) can be extracted. New utility functionsdetermineSubject
anddetermineLatestSchema
insrc/commands.schemaUtils.ts
are used to navigate from theSchemaish
argument to the underlying desired object.Any additional details or context that should be provided?
SchemaSubject
model and stop usingContainerTreeItem<Schema>
#1021, although I still useContainerTreeItem<Schema>
within the topics view hierarchy. New class issrc/models/schemas.ts
'sSubject
, and can be obtained from aSchema
instance via methodsubjectObject()
.Pull request checklist
Please check if your PR fulfills the following (if applicable):
Tests
Other
.vsix
file?From Copilot's perspective:
This pull request introduces significant changes to the schema handling functionality, including refactoring functions to use a new
SubjectishArgument
type, adding utility functions to determine schema subjects, and updating tests to reflect these changes.Key changes include:
Refactoring and Utility Functions:
src/commands/schemaUpload.ts
: RefactoreduploadSchemaForSubjectFromfile
anduploadSchemaFromFile
functions to useSubjectishArgument
and introduceddetermineSubject
for subject extraction. [1] [2] [3] [4] [5]src/commands/schemaUtils.ts
: Added new utility functionsdetermineSubject
anddetermineLatestSchema
to handle different types of subject arguments.Testing:
src/commands/schemaUtils.test.ts
: Added tests for the new utility functionsdetermineSubject
anddetermineLatestSchema
.src/loaders/loaderUtils.test.ts
: Updated tests to validate the correct order of fetched schema versions and subject strings. [1] [2]Schema Handling:
src/loaders/loaderUtils.ts
: Modified thefetchSubjects
function to returnSubject
objects instead of strings and updatedfetchSchemaSubjectGroup
to return schemas sorted by version. [1] [2] [3]src/loaders/resourceLoader.ts
: UpdatedgetTopicsForCluster
andgetSubjects
to handleSubject
objects. [1] [2] [3] [4]Command Updates:
src/commands/schemas.ts
: Updated commands to use the newdetermineLatestSchema
utility function for handling schema subjects. [1] [2] [3]These changes collectively improve the flexibility and robustness of schema handling by introducing a more versatile argument type and ensuring consistency across related functions and tests.