-
Notifications
You must be signed in to change notification settings - Fork 88
CLOUDP-322615: Add support for autoscaling mode in setup commands #3949
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
Conversation
} | ||
|
||
// newCluster creates a new cluster for the pinned API version. | ||
func (opts *Opts) newCluster() *atlasClustersPinned.AdvancedClusterDescription { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is from cluster_setup.go
, I updated it a bit to simplify but added unit tests to make sure it's behaving the same
This reverts commit 27e3e32.
APIx Bot |
internal/cli/setup/cluster_config.go
Outdated
return regionConfig | ||
} | ||
|
||
// getDiskSizeOverride returns the disk size override, which is only applicable for non-tenant clusters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultDiskSizeGB
contains values for TENANT
, so this comment is not 100% correct.
var DefaultDiskSizeGB = map[string]map[string]float64{
"TENANT": {
"M2": 2,
"M5": 5,
},
"AWS": {
"M10": 10,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, it's because based on the previous implementation, it only sets the disk size override if the provider is not tenant based on if opts.providerName() != tenant {
, let me see how I can rephrase it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the comment, let me know if it's better, if not we can remove it or you can make a suggestion! thanks
internal/cli/setup/cluster_config.go
Outdated
// getDiskSizeOverride returns the disk size override, which is only applicable for non-tenant clusters. | ||
func (opts *Opts) getDiskSizeOverride() *float64 { | ||
if opts.providerName() != tenant { | ||
diskSizeGB := defaultDiskSizeGB(opts.providerName(), opts.Tier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you pass an incorrect combination to defaultDiskSizeGB
the value returned is 0
, shouldn't we check that the value !=0
before returning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! good idea, and if it's zero we should also return nil, thank you 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated and added unit tests, thanks!
Coverage Report 📉
|
Proposed changes
Jira ticket: CLOUDP-322615
--autoScalingMode
Checklist
make fmt
and formatted my codeFurther comments