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.

Sign up for GitHub

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

Jump to bottom

master: check both places to fix bug 1372618 #11045

Merged
merged 1 commit into from
Sep 28, 2016
Merged

master: check both places to fix bug 1372618 #11045

merged 1 commit into from
Sep 28, 2016

Conversation

sosiouxme
Copy link
Member

@sosiouxme sosiouxme commented Sep 21, 2016 •
edited
Loading

@sosiouxme sosiouxme force-pushed the 20160920-bz1372618-fix-plugin-config branch from 91bf290 to f9c06a3 Compare September 21, 2016 20:59
@sosiouxme
Copy link
Member Author

@openshift/api-review seems like the best fit to review this change?

@sosiouxme
Copy link
Member Author

pretty sure the test failure had nothing to do with me

@sosiouxme
Copy link
Member Author

[test] again for fun

@sosiouxme
Copy link
Member Author

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/6409/consoleFull had a different conformance error:

• Failure [288.339 seconds]
deploymentconfigs
/data/src/github.com/openshift/origin/test/extended/deployments/deployments.go:767
  with revision history limits
  /data/src/github.com/openshift/origin/test/extended/deployments/deployments.go:716
    should never persist more old deployments than acceptable after being observed by the controller [Conformance] [It]
    /data/src/github.com/openshift/origin/test/extended/deployments/deployments.go:715

    Expected an error to have occurred.  Got:
        <nil>: nil

    /data/src/github.com/openshift/origin/test/extended/deployments/deployments.go:696

[test] once more

@sosiouxme sosiouxme mentioned this pull request Sep 27, 2016
Closed
@sosiouxme sosiouxme force-pushed the 20160920-bz1372618-fix-plugin-config branch from f9c06a3 to eb77c69 Compare September 27, 2016 14:18
@sosiouxme
Copy link
Member Author

@csrwng PTAL

csrwng
csrwng reviewed Sep 27, 2016
View reviewed changes
Copy link
Contributor

@csrwng csrwng left a comment

Choose a reason for hiding this comment

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

some style comments

pkg/cmd/server/start/start_master.go
}
}

// Look for ClusterResourceOverrideConfig in the PluginConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

fix godoc

pkg/cmd/server/start/start_master.go
}

// Look for ClusterResourceOverrideConfig in the PluginConfig
func tryOverrideConfig(ac configapi.AdmissionConfig) (*overrideapi.ClusterResourceOverrideConfig, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use a more descriptive function name?

pkg/cmd/server/start/start_master.go
@@ -527,6 +516,44 @@ func StartAPI(oc *origin.MasterConfig, kc *kubernetes.MasterConfig) error {
return nil
}

// Look in two potential places where ClusterResourceOverrideConfig can be specified
func getResourceOverrideConfig(oc *origin.MasterConfig) (*overrideapi.ClusterResourceOverrideConfig, error) {
if overrideConfig, err := tryOverrideConfig(oc.Options.AdmissionConfig); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

move the if to its own line, followed by 2 if's ...
https://golang.org/doc/effective_go.html#if

pkg/cmd/server/start/start_master.go
if oc.Options.KubernetesMasterConfig == nil { // external kube gets you a nil pointer here
return nil, nil
}
if overrideConfig, err := tryOverrideConfig(oc.Options.KubernetesMasterConfig.AdmissionConfig); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Put if in its own line, get rid of the else

pkg/cmd/server/start/start_master.go
}
if overrideConfig, err := override.ReadConfig(configFile); err != nil {
return nil, err
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

get rid of the else

@pweil-
Copy link

pweil- commented Sep 27, 2016

@mfojtik PTAL

@sosiouxme sosiouxme force-pushed the 20160920-bz1372618-fix-plugin-config branch from eb77c69 to fdedd83 Compare September 27, 2016 20:08
@sosiouxme
Copy link
Member Author

Addressed @csrwng 's feedback. Going ahead with [merge] to get it into online ASAP.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 27, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9390/) (Image: devenv-rhel7_5092)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to fdedd83

@abhgupta
Copy link
Member

re-[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to fdedd83

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9390/)

@openshift-bot openshift-bot merged commit 2af9a84 into openshift:master Sep 28, 2016
@sosiouxme sosiouxme deleted the 20160920-bz1372618-fix-plugin-config branch September 28, 2016 13:03
@sosiouxme sosiouxme mentioned this pull request Sep 28, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csrwng csrwng csrwng left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sosiouxme @pweil- @openshift-bot @abhgupta @csrwng