Skip to content

Conversation

@Similarityoung
Copy link
Contributor

This pull request refactors router configuration handling and improves the integration of application and registry context within router logic. The main changes include moving the router configuration types from the config package to a new router package, updating references throughout the codebase, and ensuring that application and registry information is consistently set and used in routing operations. These changes enhance modularity, maintainability, and correctness in routing behavior, particularly for tag-based and Polaris routers.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 18.18182% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.31%. Comparing base (60d1c2a) to head (57a01c8).
⚠️ Report is 646 commits behind head on develop.

Files with missing lines Patch % Lines
registry/directory/directory.go 12.90% 24 Missing and 3 partials ⚠️
cluster/router/polaris/router.go 0.00% 16 Missing ⚠️
cluster/router/polaris/factory.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3029      +/-   ##
===========================================
- Coverage    46.76%   40.31%   -6.46%     
===========================================
  Files          295      457     +162     
  Lines        17172    32458   +15286     
===========================================
+ Hits          8031    13086    +5055     
- Misses        8287    18107    +9820     
- Partials       854     1265     +411     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AlexStocks AlexStocks requested review from Copilot and marsevilspirit and removed request for Copilot September 20, 2025 09:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors router configuration by moving router configuration types from the config package to a new router package, and improves the integration of application and registry context within router logic.

  • Router configuration types (RouterConfig, Tag, etc.) moved from config package to router package
  • Polaris router now receives and stores application and registry context from URL attributes
  • Registry directory now sets application and registry attributes on URLs when missing

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
registry/directory/directory.go Adds logic to set application and registry attributes on URLs when missing
cluster/router/tag/router_test.go Updates imports and type references from config.RouterConfig to router.RouterConfig
cluster/router/tag/router.go Updates imports and type references to use new router package
cluster/router/tag/match.go Updates function signatures to use router.RouterConfig instead of config.RouterConfig
cluster/router/polaris/router.go Refactors to accept URL parameter and store application/registry context
cluster/router/polaris/factory.go Updates factory method to pass URL parameter to router constructor
cluster/router/config.go Defines router configuration types moved from config package

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@sonarqubecloud
Copy link

}
// set registry if not exist
if _, ok := url.GetAttribute(constant.RegistryKey); !ok {
configRegistries := config.GetRootConfig().Registries
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
configRegistries := config.GetRootConfig().Registries
// TODO: Temporary compatibility with old APIs, can be removed later
configRegistries := config.GetRootConfig().Registries

Copy link
Member

@marsevilspirit marsevilspirit left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link

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.

5 participants