Skip to content

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Feb 24, 2015

In an attempt to unravel start.go, I started by preventing the mutation of the start.config object. Once it is set by the command, nothing else mutates it and every method built upon it is a distinct call that does all the reparsing necessary to allow getters to be called in any order. I recognize this as inefficient, but it makes for straightforward to answer the question "how do I get this value?" That was the first problem I hit.

I still need to add unit tests and rebase.

@liggitt Comments on the approach before I sink more time into it? You may want to begin in start.go's start() method and imagine yourself tracing through to figure out how to add a parameter or figure out why MasterPublicAddr is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

node

@deads2k deads2k force-pushed the deads-split-master.go branch 3 times, most recently from 93d27aa to 24e3749 Compare February 24, 2015 18:12
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if addresses and certs should be sliced by server. Feels a little weird to have multiple config structs spanning the same servers.

Copy link
Contributor

Choose a reason for hiding this comment

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

make these local vars to make sure things just look at the individual start flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make these local vars to make sure things just look at the individual start flags?

turns out they are needed elsewhere. See the comment commit

@deads2k deads2k force-pushed the deads-split-master.go branch from eca21f5 to ed1e3e6 Compare February 25, 2015 18:07
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't update etcdaddr any longer

@deads2k deads2k force-pushed the deads-split-master.go branch from c75c64c to 21274f4 Compare February 25, 2015 20:37
Copy link
Contributor

Choose a reason for hiding this comment

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

use net.SplitHostPort, and handle an error condition by just returning theUrl.Host. same below for hostofhost and getport

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be using cfg.BindAddr.Host still? (or a cfg.GetNodeBindHost() which would do the same thing, but is more testable)

Copy link
Contributor

Choose a reason for hiding this comment

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

could keep using cfg.BindAddr.Host (which holds host separate from port), and drop the getHostOfHost() (or call cfg.GetEtcdBindAddr() which does the same, yadda, yadda, yadda)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it looks like we're counting on etcd defaulting to binding on 4001 when we don't specify a port. Should probably have GetEtcdBindAddress() which does net.JoinHostPort(cfg.BindAddr.Host, strconv.Itoa(cfg.EtcdAddr.DefaultPort))

@deads2k
Copy link
Contributor Author

deads2k commented Feb 26, 2015

@smarterclayton I think it's a lot safer than you'd expect. Turns out that once it's more reasonably factored, it's possible to write unit tests that cover the edge cases that gave us fits before.

Unit tests cover the defaulting and computation behavior of getters. They also cover the bindings of the command to the config itself.

I'm working on an integration test to demonstrate a method for launching an all-in-one for integration tests.

@liggitt
Copy link
Contributor

liggitt commented Feb 26, 2015

@deads2k the last bit I want to adjust/test is the --kubeconfig / --master interactions in all-in-one and node modes. After that I'll be comfortable with this. I'm working on those tests tonight.

@liggitt
Copy link
Contributor

liggitt commented Feb 26, 2015

@deads2k finished --kubeconfig stuff, added tests, restored some current behaviors (running with --kubernetes without --kubeconfig is currently allowed, etc). I squashed your commits into one and my commits into one, and spawned a new PR at #1156... wasn't sure what your schedule was.

@deads2k
Copy link
Contributor Author

deads2k commented Feb 26, 2015

replace by #1156

@deads2k deads2k closed this Feb 26, 2015
@deads2k deads2k deleted the deads-split-master.go branch February 27, 2015 12:57
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Aug 17, 2017
…service-catalog/' changes from 8f07b7b..7e650e7

7e650e7 origin build: add origin tooling
f32eec2 unit test for ./pkg/rest/core/fake rest client, addresses openshift#860 Test ready to be reviewed (openshift#1113)
e388aee explicitly always prefer latest OSBAPI Version (openshift#1138)
962429e Merge branch 'pr/1135'
b6ee7ef fix rbac
cb1beb9 Merge branch 'pr/1131'
ecc5c01 Update Code of Conduct (openshift#1137)
e7c5ab3 address one more PR comment
ddcbbad address PR comments
652a83b fix test expectation to match the new error message for missing service class
33417cc address PR comments
565fccf Use the chart name instead of the namespace (openshift#1102)
bc61919 Add new terminal failure binding condition (openshift#1057)
4e642d5 Added more detailed instructions on how to setup the repo (openshift#1114)
bdaea23 update unit tests (openshift#1123)
88a9642 validate the apiserver options (openshift#1116)
b0af5fc fix whitespace in the copyright section
dee796a generated type changes
ef585c4 Rename the directory from default to defaultservicename to conform to go style guide. Wire admission controller into the apiserver
0b5d6c6 add firewall troubleshooting section (openshift#1040)
fd9e6bc Fix Typo in Events Code of Conduct (openshift#1126)
ebe6506 Fix Typo in Terminology (openshift#1128)
0038b1e Merge branch 'pr/1122'
8411f31 make deprovisioning an instance asynchronously not fall-through to synchronous deprovision (openshift#1067)
76c1d93 handle failures from list and test the not ready condition, cleanup
9241296 finish unit tests, passing
ed75774 Minor fixes based on go report card
9911e8d Add GoReport Widget (openshift#1121)
dd24e5c clean up old cruft
08276c6 generated file changes
6489d90 Implement the default plan in admission controller
a6bb576 Code: Instance/Binding parameters from secret (openshift#1079)
10bb148 Update generated files (openshift#1115)
5291e6f v0.0.15 (openshift#1118)
28a1ea6 Merge branch 'pr/1104'
bb4a2d2 Merge branch 'pr/1097'
1c14a90 push all arch images on release tags (openshift#1108)
b587b2c Improve log output for deprovision
8887561 Remove PodPreset embedding from Binding (openshift#1030)
1abdcc8 Adjust helm/tiller installation instructions (openshift#1091)
f636f99 only skip tls verify if not behind the aggregator (openshift#1101)
43b40ab controller_broker unit test bullet-proofing openshift#1077 (openshift#1099)
bb596b8 Use data store instead of database (openshift#1100)
04fa477 Implementation: Support for Bearer token auth between Service Catalog and brokers (openshift#1053)
9e46d3c refactor Jenkins e2e tests (openshift#1082)
1f0a41e remove old/misleading comments about only doing soft delete if it's "our turn--" i.e. only if the finalizer we care about is at the head of the finalizers list.
5c1d9b8 Update OSB client (openshift#1085)
a6e80ea Only do work for instances from a single queue (openshift#1074)
2bd85d6 Merge branch 'pr/1076'
e324287 Tweaks to the walkthrough for local-up-cluster
d8b7899 Add a note to the walkthrough about getting bindings when using the aggregator (openshift#1078)
ea44cf1 msg on Environment Variables to set for e2e (openshift#1070)
d15554a Merge branch 'pr/1017'
faf966e Add comment re: async race condition in integration tests
ed2e096 v0.0.14 (openshift#1071)
fc84ffd more PR feedback
283bed4 Add integration tests and some error checking; PR feedback
903a7a7 Add terminal condition for instance and do not retry failed provisions
REVERT: 8f07b7b origin: add required patches

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: 7e650e7e39c3fc79a8ecc061cce2a70e899406ff
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.

2 participants