openshift / puppet-openshift_origin

Puppet module to create OpenShift Brokers and Nodes. Can be used to create a full OpenShift Origin deployment.
http://forge.puppetlabs.com/openshift/openshift_origin
Other
45 stars 128 forks source link

Add extra district, zone and region options #285

Closed andrewklau closed 9 years ago

detiber commented 10 years ago

@andrewklau outside my one comment that conf_broker_require_districts should default to true :+1: on the changes. Please rebase so I can merge.

andrewklau commented 10 years ago

@detiber I left conf_broker_require_districts as false as not to add extra complications for new users who would start with oo-install.

detiber commented 10 years ago

@andrewklau oo-install now creates a default district and adds the node(s) to that district.

The bigger issue would be users that start out with a non-districted node, create applications and then want to move to a district.

andrewklau commented 10 years ago

@detiber ok, trying to rebase it now

andrewklau commented 10 years ago

@detiber I am awkwardly having problems rebasing this.. tips?

detiber commented 10 years ago

@andrewklau just tried a checkout of your PR, did a git rebase -i upstream/master (with upstream being my remote for this repo, instead of my fork) and outside of a conflict to resolve in README.asciidoc it looks to do the right thing (I only see a single commit afterwards for Add extra district, zone and region options)

detiber commented 10 years ago

@andrewklau it probably doesn't help that I was merging some other requests that may have muddied the waters a bit as you were attempting the rebase as well.

andrewklau commented 10 years ago

@detiber ok I think I worked it out :)

detiber commented 10 years ago

[test]

openshift-bot commented 10 years ago

Origin Test Results: FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests/3209/)

detiber commented 10 years ago

Looks like we'll need to update the vagrant plugin to set conf_broker_require_districts to false for this. I'll try to verify that change along with this one next week since it'll require some manual testing. If I manage to forget about it, ping me on it.

andrewklau commented 10 years ago

oops looks like we both forgot -- ping @detiber -c 1

detiber commented 10 years ago

@andrewklau will be a few days before I can revisit. Possibly @sdodson may be able to do the necessary testing before I can get back to it.

detiber commented 10 years ago

@andrewklau things have been pretty hectic this past month or so, still have this on my radar to verify, but will probably still be another couple of weeks before I get to it. Unless @nhr or @sdodson are able to review before I get a chance.

detiber commented 10 years ago

@andrewklau just a heads up, I've started testing on this (and the associated vagrant-openshift change required) and have been running into some complications related to the age of the dependencies image we use for vagrant.

I should be able to finish up testing for this in the next couple of days.

detiber commented 10 years ago

@andrewklau just finished verifying this with the associated vagrant change: https://github.com/openshift/vagrant-openshift/pull/141 and everything looks good. Once you get a chance to rebase, I will get both of the PRs merged.

andrewklau commented 9 years ago

@detiber sorry! I didn't see your comment so many days ago. I've rebased it now

sdodson commented 9 years ago

[test]

sdodson commented 9 years ago

[test] now that requisite vagrant-openshift changes have landed.

openshift-bot commented 9 years ago

Evaluated for origin up to 2b768965617d5c377ee5393e09fbf626793716a6

Filirom1 commented 9 years ago

Thanks !