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

fix: renaming standaloneDeployment variable to be consistent #5183

Closed
wants to merge 1 commit into from
Closed

fix: renaming standaloneDeployment variable to be consistent #5183

wants to merge 1 commit into from

Conversation

TresTristesTrigues
Copy link

This PR fixes 2 issues:

  1. There was a mismatch with variable naming in the Flyte-Core chart, where standalone_deploy and standaloneDeployment were being used interchangeably
  2. cluster_resource_manager could be deployed twice as part of flyteadmin and standalone

Copy link

welcome bot commented Apr 4, 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).

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Apr 4, 2024
Copy link

codecov bot commented Apr 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.08%. Comparing base (452538a) to head (17d1eff).
Report is 802 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5183      +/-   ##
==========================================
+ Coverage   59.06%   59.08%   +0.01%     
==========================================
  Files         646      646              
  Lines       55726    55739      +13     
==========================================
+ Hits        32916    32934      +18     
+ Misses      20214    20209       -5     
  Partials     2596     2596              
Flag Coverage Δ
unittests 59.08% <ø> (+0.01%) ⬆️

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.

@neverett neverett requested a review from davidmirror-ops April 8, 2024 14:18
@neverett
Copy link
Contributor

neverett commented Apr 8, 2024

cc @davidmirror-ops to review the charts

Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

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

cluster_resource_manager could be deployed twice as part of flyteadmin and standalone

Do you think this is still the case with the changes you're proposing?

Thanks!

@TresTristesTrigues
Copy link
Author

@davidmirror-ops
I am not sure if it should be possible to deploy it has both standalone and flyte-admin, I assume not.
In that case, should this line be changed to:

{{- if and (.Values.cluster_resource_manager.enabled) (.Values.cluster_resource_manager.standaloneDeployment) }}

? (in contrast to this line)

Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

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

Sorry for the delays. Thank you!

@TresTristesTrigues TresTristesTrigues closed this by deleting the head repository Sep 24, 2024
@davidmirror-ops
Copy link
Contributor

@TresTristesTrigues sorry, I think we should have merged this. Any chance we can still make it happen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants