redhat-openstack / ansible-role-tripleo-image-build

An ansible role used to create and publish quickstart built images
Apache License 2.0
5 stars 3 forks source link

scope variables with a prefix #1

Closed halcyondude closed 8 years ago

halcyondude commented 8 years ago

all variables need to be scoped to this role, so we don't pollute the global namespace, and to ensure we don't break suddenly when someone uses a common name that we happen to use here. See here for more details (https://galaxy.ansible.com/intro#variables). Since having variables names like "ansible-role-tripleo-image-build-overcloud_overwrite_existing" would be silly, using the following prefix is not (in comparison):

artib ("ansible-role-tripleo-image-build")

so... "artib_overcloud_overwrite_existing" isn't wonderful, but would suffice.

halcyondude commented 8 years ago

@weshayutin thoughts?

trown commented 8 years ago

+1 https://review.openstack.org/#/c/304860/ shows we already have a conflict without namespacing.

We will need to do some coordination when merging this though, since I think tripleo-quickstart will be switched over to using this role before the namespace change merges.

halcyondude commented 8 years ago

@trown - ok. The changes you're making on your fork are primarily in adding repo support for rdo + newton via https://github.com/redhat-openstack/ansible-role-tripleo-image-build/tree/master/templates right? I'll post the review to gerrit and we can coordinate. Happy happy to deal with the rebase if you beat me there :)

trown commented 8 years ago

@halcyondude the only change in tripleo-quickstart is to switch to using this role. All of the repo support stuff I will submit to this role.

It just that to even add support for using this role externally I have to override the working_dir variable on role import: https://review.openstack.org/#/c/304860

So if we change the name of that variable, without changing tripleo-quickstart to do something better, kaboom! Should be easy enough to coordinate, we just don't have the benefit of depends-on because of 2 different gerrits.

trown commented 8 years ago

@halcyondude actually I wonder if we could not just agree on default for new artib_working_dir option. Then it would just work in the tripleo-quickstart case.

/home/oooq-images is a bit odd according to FHS

/var/lib/oooq-images seems appropriate for the content we put there.

halcyondude commented 8 years ago

@trown re: FHS, +1 makes sense.

I like "artib_" as the prefix, it scales for the other roles (which don't yet do namespacing either) such as

"artup_" --> https://github.com/redhat-openstack/ansible-role-tripleo-undercloud-post

/me predicts Ansible is going to get variable default namespace support in a future version

in the meantime...

halcyondude commented 8 years ago

I'm not crazy about having to do this at all...but this seems workable.

@weshayutin @harryrybacki thoughts?

artib --> https://github.com/redhat-openstack/ansible-role-tripleo-image-build artip --> https://github.com/redhat-openstack/ansible-role-tripleo-image-publish arto --> https://github.com/redhat-openstack/ansible-role-tripleo-overcloud artou --> https://github.com/redhat-openstack/ansible-role-tripleo-overcloud-upgrade artup --> https://github.com/redhat-openstack/ansible-role-tripleo-undercloud-post arttu --> https://github.com/redhat-openstack/ansible-role-tripleo-tempest-undercloud

trown commented 8 years ago

my only concern is this hashing algorithm is not very future proof wrt collisions, since all of the roles we are thinking of would be art[a-z]{1,2} that is a pretty small space. In reality it is much smaller than that because we aren't picking random letters but the first letter of a English (or sort of English) word.

trown commented 8 years ago

maybe that is a problem for the first role that has the same initials as a previously used one though.

halcyondude commented 8 years ago

@trown breaking ground on this now

halcyondude commented 8 years ago

https://review.gerrithub.io/#/c/273144

halcyondude commented 8 years ago

@trown this is the change that needs to be coordinated for working_dir --> artib_working_dir

I agree w/ FHS comments above, but want to keep this issue (and commit) a simple variable rename w/ prefix. we can change default in a discreet patch.

halcyondude commented 8 years ago

Closing as fixed. https://github.com/redhat-openstack/ansible-role-tripleo-image-build/commit/e22ae3607d8284555369e0f604e33b6a14094cb6