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

[copilot] rename sidecar to uploader #6043

Merged

Conversation

wayner0628
Copy link
Contributor

@wayner0628 wayner0628 commented Nov 21, 2024

Tracking issue

#5995

Why are the changes needed?

The current naming of the upload container in Copilot as sidecar can be confusing, especially since we also have a dedicated downloader container. Given that this container primarily handles upload tasks, renaming it to something more descriptive would improve readability and understanding of its role.

What changes were proposed in this pull request?

Replace all occurences of sidecar with uploader.
Simply change the string value here, suggested by @eapolinario (thank you), because replacing all occurences will introduce too many changes across the codebase.
Also, for flyte users, they will only see the name of container.

How was this patch tested?

Setup process

Screenshots

  • k8s
    Screenshot 2024-11-21 at 11 18 20 AM
  • unit test
Screenshot 2024-11-25 at 9 48 33 PM

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Signed-off-by: wayner0628 <[email protected]>
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.04%. Comparing base (e13babb) to head (aa9ae72).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6043      +/-   ##
==========================================
- Coverage   37.06%   37.04%   -0.03%     
==========================================
  Files        1316     1316              
  Lines      132191   132262      +71     
==========================================
- Hits        49002    48998       -4     
- Misses      78926    79002      +76     
+ Partials     4263     4262       -1     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.10% <ø> (+0.02%) ⬆️
unittests-flytecopilot 22.23% <ø> (ø)
unittests-flytectl 62.46% <ø> (-0.05%) ⬇️
unittests-flyteidl 7.23% <ø> (-0.02%) ⬇️
unittests-flyteplugins 53.73% <ø> (+0.01%) ⬆️
unittests-flytepropeller 42.63% <ø> (-0.03%) ⬇️
unittests-flytestdlib 57.59% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Signed-off-by: wayner0628 <[email protected]>
Signed-off-by: wayner0628 <[email protected]>
@wayner0628 wayner0628 marked this pull request as ready for review November 21, 2024 19:23
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Thank you. Can you add a test to copilot_test.go to verify the container name change?

Signed-off-by: wayner0628 <[email protected]>
@wayner0628
Copy link
Contributor Author

@eapolinario test added, thank you!

Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

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

thank you

Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

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

thank you

@Future-Outlier Future-Outlier merged commit 34e48d5 into flyteorg:master Dec 2, 2024
51 of 52 checks passed
@wayner0628 wayner0628 deleted the rename-sidecar-to-uploader branch December 2, 2024 04:43
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.

3 participants