adding exclude files + other changes to get registration to work for me #2036
Conversation
|
👋 patrickhuie19, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
| // So if the import is "workflows/v1/metadata.proto", the name should be "v1/metadata.proto" | ||
| // And if the import is "workflows/v2/cre_info.proto", the name should be "v2/cre_info.proto" | ||
| importName := dep | ||
| if strings.HasPrefix(dep, "workflows/") { |
There was a problem hiding this comment.
this will only work with current folder structure. That's why I was removing that prefix dynamically. I'd rather not hardcode that workflows/ removal and keep it dynamic.
There was a problem hiding this comment.
mm. i'm the owner of those protos and I don't see a reason to change the folder structure.
|
|
||
| // Check if schema is already registered | ||
| if existingID, exists := checkSchemaExists(schemaRegistryURL, subject); exists { |
There was a problem hiding this comment.
maybe we should do that before if depSubject, depExists := subjectMap[dep]; depExists {?
|
|
||
| // Check if schema is already registered | ||
| if existingID, exists := checkSchemaExists(schemaRegistryURL, subject); exists { | ||
| framework.L.Info().Msgf("Schema %s already exists with ID %d, skipping registration", subject, existingID) |
| // The schema registry expects import statements without the 'workflows/' prefix | ||
| // So we need to modify the protobuf content to replace "workflows/v1/..." with "v1/..." and "workflows/v2/..." with "v2/..." | ||
| modifiedSchema := strings.ReplaceAll(schema.Source, `"workflows/v1/`, `"v1/`) | ||
| modifiedSchema = strings.ReplaceAll(modifiedSchema, `"workflows/v2/`, `"v2/`) |
| if _, exists := protoMap[importPath]; exists { | ||
| // Check for self-reference | ||
| if importPath == path { | ||
| framework.L.Warn().Msgf("Self-reference detected: %s imports itself!", path) |
There was a problem hiding this comment.
would that mean that proto is invalid or our code is buggy? if it is the former, maybe we should err?
There was a problem hiding this comment.
potentially either - but now that we parse the directories dynamically and strip the suffixes, it could also be because there are overlapping paths at one level down: i.e.,
/workflows/event_started.proto
/capabilities/event_started.proto
continuing here allows us to register the rest of the protos; that seems like the best middle ground for how stable the framework is atm.
This pull request introduces support for excluding specific proto files from registration and refactors the proto registration workflow to use proper topological sorting based on file dependencies. It also improves logging and simplifies code paths for proto file fetching and caching.
Proto file exclusion support:
ExcludeFilesfield to theProtoSchemaSetstruct, allowing users to specify proto files to exclude from registration. The exclusion logic is now applied when fetching proto files from both GitHub and the local filesystem. (framework/components/dockercompose/chip_ingress_set/protos.go, [1] [2] [3]Dependency-aware registration:
framework/components/dockercompose/chip_ingress_set/protos.go, framework/components/dockercompose/chip_ingress_set/protos.goL425-R572)framework/components/dockercompose/chip_ingress_set/protos.go, framework/components/dockercompose/chip_ingress_set/protos.goL425-R572)Improved proto file fetching and caching:
ExcludeFilesparameter, ensuring excluded files are not processed or cached. (framework/components/dockercompose/chip_ingress_set/protos.go, [1] [2] [3] [4]framework/components/dockercompose/chip_ingress_set/protos.go, framework/components/dockercompose/chip_ingress_set/protos.goL319-L327)Logging and minor improvements:
framework/components/dockercompose/chip_ingress_set/protos.go, [1] [2]framework/components/dockercompose/chip_ingress_set/protos.go, [1] [2]