Skip to content

Conversation

@liggitt
Copy link
Contributor

@liggitt liggitt commented Feb 26, 2015

Extension of #1128

@smarterclayton this is worthwhile to get in now:

  • makes the 11,000,000 different ways of running openshift start testable
  • lets you trigger external kubernetes mode with either --kubernetes=... or --kubeconfig=...
  • prevents credentials in --kubeconfig from being sent to the wrong server if --kubernetes was also set and pointed somewhere else
  • lays the groundwork for separating start into stages later (input stage, host calculation/defaulting stage, bootstrapping/cert generation stage, execute stage)

@liggitt
Copy link
Contributor Author

liggitt commented Feb 26, 2015

[test]

@openshift-bot
Copy link
Contributor

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

@liggitt
Copy link
Contributor Author

liggitt commented Feb 26, 2015

@deads2k review my commits?

@deads2k
Copy link
Contributor

deads2k commented Feb 26, 2015

Well, my schedule definitely didn't include 2am... :)

@deads2k
Copy link
Contributor

deads2k commented Feb 26, 2015

I like it. I just want to make sure I can launch it in an integration test and then I'd say it's good.

@smarterclayton
Copy link
Contributor

On Feb 25, 2015, at 11:24 PM, Jordan Liggitt [email protected] wrote:

Extension of #1128

@smarterclayton this is worthwhile to get in now:

makes the 11,000,000 different ways of running openshift start testable
Citation needed
lets you trigger external kubernetes mode with either --kubernetes=... or --kubeconfig=...
prevents credentials in --kubeconfig from being sent to the wrong server if --kubernetes was also set and pointed somewhere else
lays the groundwork for separating start into stages later (input stage, host calculation/defaulting stage, bootstrapping/cert generation stage, execute stage)
You can view, comment on, or merge this pull request online at:

#1156

Commit Summary

make start.config immutable
Rework --kubeconfig handler, misc tweaks
Ensure create of master policy namespace happens after policy will allow it
File Changes

M hack/test-end-to-end.sh (4)
M pkg/cmd/openshift/openshift.go (3)
A pkg/cmd/server/command.go (129)
A pkg/cmd/server/command_test.go (304)
A pkg/cmd/server/config.go (299)
A pkg/cmd/server/config_test.go (418)
M pkg/cmd/server/crypto/crypto.go (16)
A pkg/cmd/server/kube_master.go (60)
A pkg/cmd/server/node_config.go (37)
A pkg/cmd/server/origin/config.go (208)
M pkg/cmd/server/origin/master.go (113)
A pkg/cmd/server/origin_master.go (424)
M pkg/cmd/server/start.go (825)
D pkg/cmd/server/start_test.go (34)
Patch Links:

https://github.com/openshift/origin/pull/1156.patch
https://github.com/openshift/origin/pull/1156.diff

Reply to this email directly or view it on GitHub.

@deads2k
Copy link
Contributor

deads2k commented Feb 26, 2015

Citation needed

Authorization integration tests are needed and this fix makes it easy to do. By clearly splitting responsibilities in start.go, we were able to write a series of unit tests to catch edges, both on general defaulting and on the bindings of those config values to the command line.

@smarterclayton
Copy link
Contributor

That was a joke on the 11 million

On Feb 26, 2015, at 6:13 AM, David Eads [email protected] wrote:

Citation needed

Authorization integration tests are needed and this fix makes it easy to do. By clearly splitting responsibilities in start.go, we were able to write a series of unit tests to catch edges, both on general defaulting and on the bindings of those config values to the command line.


Reply to this email directly or view it on GitHub.

@smarterclayton
Copy link
Contributor

See comments, otherwise this looks like an excellent refactoring, thank you David and Jordan

deads2k and others added 4 commits February 26, 2015 09:41
Require --kubernetes host and --kubeconfig host to match

Shorten --write-config flag name

Limit kubeconfig loading to --kubeconfig flag only (no env or $HOME lookup)

Change node to use --kubernetes address (which falls back to --master if unspecified)

Restore previous behaviors:
 * default to local IP as --master, even for start node
 * allow running with --kubernetes without --kubeconfig

Derive etcd bind addr

Make cert generation less chatty

Fix openshift-deployer cert name
@liggitt
Copy link
Contributor Author

liggitt commented Feb 26, 2015

comments addressed, squashed as much as I plan to (david's together for credit/blame, mine together for credit/blame, two small unrelated fixes as separate commits)

@deads2k
Copy link
Contributor

deads2k commented Feb 26, 2015

lgtm [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1048/) (Image: devenv-fedora_904)

@deads2k
Copy link
Contributor

deads2k commented Feb 26, 2015

the test worked, probably a transient. re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 35ff8be

openshift-bot pushed a commit that referenced this pull request Feb 26, 2015
@openshift-bot openshift-bot merged commit 32e151d into openshift:master Feb 26, 2015
@liggitt liggitt deleted the start_config branch February 26, 2015 18:12
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Sep 5, 2017
…service-catalog/' changes from 7e650e7e39..ef63307bdb

ef63307bdb origin build: add origin tooling
a876fe3 v0.0.17 (openshift#1178)
c5237fe correct osbapi service definition (openshift#1177)
6036d4e Adding walkthrough instructions for 1.7 (openshift#1171)
5f111dd Specifying that you need Helm v2.5.0 for installation (openshift#1170)
08043bd Adding more small fixes to the walkthrough & install docs (openshift#1169)
d65d4a1 rbac targets needed to be renamed as well (openshift#1161)
590f6f2 Write helm command to file for api aggregation (openshift#1141)
49ddcf6 clean before building a specific arch (openshift#1168)
43f7cfb Splitting up the Walkthrough for 1.6 and 1.7 instructions (openshift#1163)
02e0217 Updates to README (openshift#1166)
57f2aa5 Adding instructions for installing from Macs (openshift#1164)
dfe620e fix rate-limiting for polling queue (openshift#1143)
ca5f335 Use Generation instead of checksum for Broker (openshift#1145)
5364daa Merge branch 'pr/1158'
f34c5db move Travis deployment script to directory in 'contrib/'
2a00d7f Update incorrect port (openshift#1156)
b0ed60e improve the repository's layout (openshift#1154)
f870baf Follow up file / renames from openshift#1142 (openshift#1152)
826b4f9 remove unnecessary json annotations (openshift#1153)
33cb345 Rename resources. closes openshift#1080 (openshift#1142)
70c2b9b Add ability to specify CA certs to use for TLS authentication. (openshift#1112)
2aa5039 v0.0.16 (openshift#1140)
65de49c Comments for unit test bullet proofing (openshift#1139)
REVERT: 7e650e7e39 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: ef63307bdbaa64efca204912f5361a4f3d3be2c8
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Sep 11, 2017
…service-catalog/' changes from 7e650e7e39..ef63307bdb

ef63307bdb origin build: add origin tooling
a876fe3 v0.0.17 (openshift#1178)
c5237fe correct osbapi service definition (openshift#1177)
6036d4e Adding walkthrough instructions for 1.7 (openshift#1171)
5f111dd Specifying that you need Helm v2.5.0 for installation (openshift#1170)
08043bd Adding more small fixes to the walkthrough & install docs (openshift#1169)
d65d4a1 rbac targets needed to be renamed as well (openshift#1161)
590f6f2 Write helm command to file for api aggregation (openshift#1141)
49ddcf6 clean before building a specific arch (openshift#1168)
43f7cfb Splitting up the Walkthrough for 1.6 and 1.7 instructions (openshift#1163)
02e0217 Updates to README (openshift#1166)
57f2aa5 Adding instructions for installing from Macs (openshift#1164)
dfe620e fix rate-limiting for polling queue (openshift#1143)
ca5f335 Use Generation instead of checksum for Broker (openshift#1145)
5364daa Merge branch 'pr/1158'
f34c5db move Travis deployment script to directory in 'contrib/'
2a00d7f Update incorrect port (openshift#1156)
b0ed60e improve the repository's layout (openshift#1154)
f870baf Follow up file / renames from openshift#1142 (openshift#1152)
826b4f9 remove unnecessary json annotations (openshift#1153)
33cb345 Rename resources. closes openshift#1080 (openshift#1142)
70c2b9b Add ability to specify CA certs to use for TLS authentication. (openshift#1112)
2aa5039 v0.0.16 (openshift#1140)
65de49c Comments for unit test bullet proofing (openshift#1139)
REVERT: 7e650e7e39 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: ef63307bdbaa64efca204912f5361a4f3d3be2c8
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants