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

[languagedetection] Refactor languagemodels package #33356

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

davidor
Copy link
Member

@davidor davidor commented Jan 24, 2025

What does this PR do?

This pull request introduces a refactoring opportunity I identified while working on related tasks.

While working on the workloadmeta definition package, I noticed that it depended on both pkg/languagedetection/languagemodels and pkg/languagedetection/util (the latter further depends on pkg/util/kubernetes).

From the perspective of the workloadmeta store, we only need some types, so I expected them to reside in pkg/languagedetection/languagemodels. However, they were actually scattered between pkg/languagedetection/languagemodels and pkg/languagedetection/util, with some duplicate types, such as "LanguageName".

This pull request consolidates all the types/models from pkg/languagedetection/util into pkg/languagedetection/languagemodels for the sake of clarity.

Describe how you validated your changes

This refactor should be covered by tests.

@davidor davidor added changelog/no-changelog qa/done QA done before merge and regressions are covered by tests team/container-platform The Container Platform Team labels Jan 24, 2025
@davidor davidor added this to the 7.64.0 milestone Jan 24, 2025
@davidor davidor requested review from a team as code owners January 24, 2025 14:30
@davidor davidor requested a review from paullegranddc January 24, 2025 14:30
@github-actions github-actions bot added the long review PR is complex, plan time to review it label Jan 24, 2025
Copy link
Contributor

@guyarb guyarb left a comment

Choose a reason for hiding this comment

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

LGTM for the USM owned files

Copy link
Contributor

@adel121 adel121 left a comment

Choose a reason for hiding this comment

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

Thanks for this refactor 😄

@davidor
Copy link
Member Author

davidor commented Jan 31, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 31, 2025

Devflow running: /merge

View all feedbacks in Devflow UI.


2025-01-31 11:54:49 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 26m.


2025-01-31 12:23:59 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue dd-mergequeue bot merged commit cd041a2 into main Jan 31, 2025
275 checks passed
@dd-mergequeue dd-mergequeue bot deleted the davidor/refactor-languagemodels branch January 31, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog long review PR is complex, plan time to review it qa/done QA done before merge and regressions are covered by tests team/container-platform The Container Platform Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants