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

beacuse flyte-binary image has no version tag, it is replaced with fl… #5749

Conversation

sorushsaghari
Copy link

Tracking issue

Related to the discussion in Slack about the naming of the Flyte Docker image tag.

Why are the changes needed?

The current Docker image flyte-binary does not have a stable version tag, which leads to confusion during deployments. To prevent this, the image name is being changed to flyte-binary-release, as agreed upon in the discussion.

What changes were proposed in this pull request?

This PR proposes changing the Docker image name from flyte-binary to flyte-binary-release in the Flyte Helm chart. This change ensures that a stable version tag is consistently used, reducing confusion and aligning with the team's agreed standards.

Changes:

  • Updated the Flyte Helm chart to use cr.flyte.org/flyteorg/flyte-binary-release as the image repository.
  • Set the image tag to latest and the pull policy to IfNotPresent.

How was this patch tested?

The changes were verified through updates to the Flyte Helm chart configuration. Further testing includes:

  • Deployment testing with the updated Helm chart to ensure the correct image is pulled.
  • Validation of the stable tag behavior by deploying the Helm chart with the new flyte-binary-release image.

Setup process

  1. Updated the Helm chart configuration with the new image repository and tag.
  2. Deployed the updated Helm chart to a test environment.
  3. Verified the deployment success and checked the pulled image version.

Screenshots

N/A

Check all the applicable boxes

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

Related PRs

None

Copy link

welcome bot commented Sep 14, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

…yte-binary-release for prevent confusin

Signed-off-by: s.saghari <[email protected]>
Signed-off-by: sorush saghari <[email protected]>
@davidmirror-ops
Copy link
Contributor

@eapolinario would this change have any impact on the release process?

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.36%. Comparing base (a2957cf) to head (55a4db7).
Report is 63 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5749   +/-   ##
=======================================
  Coverage   36.36%   36.36%           
=======================================
  Files        1304     1304           
  Lines      110145   110145           
=======================================
  Hits        40056    40056           
  Misses      65920    65920           
  Partials     4169     4169           
Flag Coverage Δ
unittests-datacatalog 51.37% <ø> (ø)
unittests-flyteadmin 55.60% <ø> (ø)
unittests-flytecopilot 12.17% <ø> (ø)
unittests-flytectl 62.21% <ø> (ø)
unittests-flyteidl 7.14% <ø> (ø)
unittests-flyteplugins 53.45% <ø> (ø)
unittests-flytepropeller 42.04% <ø> (ø)
unittests-flytestdlib 55.37% <ø> (ø)

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.

@sorushsaghari
Copy link
Author

@davidmirror-ops can you help me with the failed tests?

@davidmirror-ops
Copy link
Contributor

@sorushsaghari sure! please run make helm from your fork, and push the changes

@sorushsaghari
Copy link
Author

@sorushsaghari sure! please run make helm from your fork, and push the changes

after the make helm i heve some merge conflicts

Signed-off-by: sorush saghari <[email protected]>
@sorushsaghari sorushsaghari force-pushed the flyte-binary-default-image-fix branch from b876063 to ff50e87 Compare September 29, 2024 11:05
@sorushsaghari
Copy link
Author

@davidmirror-ops could you please help me with this. i fixed the conflicts

@davidmirror-ops davidmirror-ops enabled auto-merge (squash) October 17, 2024 10:42
@sorushsaghari
Copy link
Author

@eapolinario can you pls merge it ? it is taking so much time and, again i have some conflicts

@eapolinario
Copy link
Contributor

Unfortunately, this is not the correct fix to version the flyte-binary image. I opened #6010 with the proper fix.

auto-merge was automatically disabled November 14, 2024 04:45

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants