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

#4544 - Fix inadvertent whitespace in v1beta3 enablement #4735

Merged
merged 1 commit into from
Feb 25, 2015
Merged

#4544 - Fix inadvertent whitespace in v1beta3 enablement #4735

merged 1 commit into from
Feb 25, 2015

Conversation

screeley44
Copy link
Contributor

This was an attempt to fix an issue that I discovered when enabling v1beta3 in the apiserver config. I accidentally had an extra trailing whitespace in the KUBE_API_ARGS and v1beta3 was never recognized. I tried to find a way to trim this value when it's loaded into the api server but was not successful. So the proposed solution will avoid inadvertent white spaces in the --runtime_config= string.

@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

//avoid inadvertent white spaces in the apiserver config file
//i.e. KUBE_API_ARGS="--runtime_config=api/v1beta3 "
v1beta3 := false
for k, _ := range s.RuntimeConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, but this isn't the right "fix" for this problem. It needs to be addressed in pkg/util/configuration_map.go, by banning whitespace in key names.

@screeley44
Copy link
Contributor Author

@bgrant0607 - yeah was looking at that originally and for some reason I could not strip any of the white space in the key names, let me try again and see if I am more successful, maybe I was stripping the value and not the key

@screeley44
Copy link
Contributor Author

@bgrant0607 - so when I originally was messing with this on Friday, I wasn't trimming the else, I think this will solve the problem for efficiently. Not sure how to update a Pull Request, is that even possible or do I need to create a new pull request?

func (m _ConfigurationMap) Set(value string) error {
for _, s := range strings.Split(value, ",") {
if len(s) == 0 {
continue
}
arr := strings.SplitN(s, "=", 2)
if len(arr) == 2 {
(_m)[strings.TrimSpace(arr[0])] = strings.TrimSpace(arr[1])
} else {
(*m)[strings.TrimSpace(arr[0])] = ""
}
}
return nil
}

@bgrant0607
Copy link
Member

Pushing to the same branch updates the PR automatically. Will take a look, thanks.

@@ -205,7 +205,8 @@ func (s *APIServer) Run(_ []string) error {
glog.Fatalf("Failure to start kubelet client: %v", err)
}

_, v1beta3 := s.RuntimeConfig["api/v1beta3"]
_, v1beta3 := s.RuntimeConfig["api/v1beta3"]
Copy link
Member

Choose a reason for hiding this comment

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

You introduced a space here by accident.

@bgrant0607
Copy link
Member

LGTM.

@bgrant0607 bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 24, 2015
@bgrant0607
Copy link
Member

Please fix the space and squash your commits.

@screeley44
Copy link
Contributor Author

@bgrant0607 - so I think I finally figured out how to reset the other failed commits and "squash"into a single file commit (which I intended all along). Thanks for your patience and let me know if I still have something that I need to do

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2015
@bgrant0607
Copy link
Member

Thanks. Have you completed the CLA, or do you have a corporate CLA? If the latter, what company are you with?

@screeley44
Copy link
Contributor Author

@bgrant0607 - I signed the individual CLA, not sure about the corporate CLA, but I'm with Redhat.

@googlebot
Copy link

CLAs look good, thanks!

vishh added a commit that referenced this pull request Feb 25, 2015
#4544 - Fix inadvertent whitespace in v1beta3 enablement
@vishh vishh merged commit aaa4a35 into kubernetes:master Feb 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants