Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Configuration flags for cluster autoscaler #805

Merged
merged 1 commit into from
Apr 19, 2016

Conversation

mwielgus
Copy link
Contributor

@mwielgus mwielgus commented Apr 19, 2016

func (this *MigConfigFlag) String() string {
var b bytes.Buffer
b.WriteString("[")
for i, npc := range *this {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is npc? Please use more descriptive names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


type MigConfigFlag []MigConfig

func (this *MigConfigFlag) String() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use combination of fmt.Sprintf and strings.Join? It'll be much more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mwielgus mwielgus force-pushed the flags branch 3 times, most recently from c43b8d0 to d258ef0 Compare April 19, 2016 14:18
} else {
return fmt.Errorf("failed to parse mig url: %s", tokens[2])
}
if len(migconfig.Url.String()) < 5 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment why 5.

@fgrzadkowski fgrzadkowski added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2016
@mwielgus
Copy link
Contributor Author

Ref: kubernetes/kubernetes#24404

@mwielgus mwielgus merged commit 70bc1f6 into kubernetes-retired:master Apr 19, 2016
mwielgus added a commit to kubernetes/autoscaler that referenced this pull request Apr 18, 2017
Configuration flags for cluster autoscaler
mwielgus added a commit to kubernetes/autoscaler that referenced this pull request Apr 18, 2017
Configuration flags for cluster autoscaler
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants