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

chore: refactor some files from the api directory to internal #2808

Closed
wants to merge 1 commit into from

Conversation

hanxiaop
Copy link
Contributor

No description provided.

@hanxiaop hanxiaop requested a review from a team as a code owner October 22, 2024 08:25
Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit 014bdac
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/67176267bdab330008fcc9f9
😎 Deploy Preview https://deploy-preview-2808--docs-kargo-io.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: xiaopeng <hanxiaop8@outlook.com>
@hanxiaop hanxiaop requested a review from krancour October 22, 2024 08:29
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 73.56322% with 69 lines in your changes missing coverage. Please review.

Project coverage is 48.68%. Comparing base (cf12780) to head (014bdac).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
internal/helpers/stage_helpers.go 71.60% 21 Missing and 2 partials ⚠️
internal/helpers/promotion_helpers.go 73.17% 21 Missing and 1 partial ⚠️
internal/controller/promotions/promotions.go 75.00% 4 Missing ⚠️
internal/controller/stages/stages.go 86.36% 3 Missing ⚠️
internal/helpers/warehouse_helpers.go 40.00% 3 Missing ⚠️
cmd/controlplane/api.go 0.00% 1 Missing ⚠️
internal/api/abort_promotion.go 0.00% 1 Missing ⚠️
internal/api/abort_verification_v1alpha1.go 0.00% 1 Missing ⚠️
internal/api/approve_freight_v1alpha1.go 66.66% 1 Missing ⚠️
internal/api/promote_to_stage_v1alpha1.go 66.66% 1 Missing ⚠️
... and 9 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2808      +/-   ##
==========================================
- Coverage   48.83%   48.68%   -0.15%     
==========================================
  Files         270      272       +2     
  Lines       23932    23962      +30     
==========================================
- Hits        11687    11666      -21     
- Misses      11616    11663      +47     
- Partials      629      633       +4     

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

@krancour
Copy link
Member

What is the goal of this PR? I would never make a claim that everything is organized 100% optimally as is, but these changes move a lot of important functionality out of a package where it made sense for that functionality to live into a nondescript "helpers" package. Helper packages are almost always a code smell. With some careful thought, there is almost always a better answer than moving functionality into a package whose name does not reflect a clear purpose. It becomes a virtual certainty that such a package will become a "junk drawer" over time.

@hanxiaop
Copy link
Contributor Author

What is the goal of this PR?

@krancour sorry for not writing the description and causing confusion. I agree that helpers is not a great name for the moved files. I was trying to move just event.go file to a more general event directory that includes both kargo related events and k8s events, which could also unblock PR #2789. However, I realized this isn’t possible since the functionality in those helper files in the kargo api direcotry also rely on some functionality in that file. I think it would be better to separate the client-related functionality from the API definitions to improve modularity and reusability.

@krancour
Copy link
Member

I agree with event-related things moving, especially if it disrupts a would-be import cycle, but there are definitely some things moved around in here that, as far as I can tell, don't need to move. Things like GetStage() or GetPromotion() look like they would have been fine remaining where they were. Or am I missing something?

@hanxiaop
Copy link
Contributor Author

Those functionalities you mentioned are coupled with some functions in event.go, but it is not a mandatory refactor as I was just trying to separate useful functionality from the API definitions. I'll close this PR and go with your suggestion in #2789 (comment) with the minimum refactor. Thank you for the review!

@hanxiaop hanxiaop closed this Oct 23, 2024
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.

2 participants