aeolus-incubator / tim

Web application for managing virtual images to be deployed in the cloud
www.aeolusproject.org
MIT License
3 stars 6 forks source link

Templates and nesting fixes #44

Closed mtaylor closed 12 years ago

mtaylor commented 12 years ago

You should now be able to create a Base Image with a nested Template, ImageVersion and TargetImage. Example XML shown below.

https://gist.github.com/3865310

jguiditta commented 12 years ago

OK, so I tested this building a mock image, using the xml in the gist. However, factory requires the os to have a name of something like 'RHELMock' or 'FedoraMock' in addition to the target type setting we have. However, when I changed the template to have that, then it fails validation, because the rng in this patch has an explicit list of valid names of operating systems (which does not include anything with Mock). Comparing ours and the official tdl in the oz repo, the official one has no such explicit list.

So, I dropped in the valid one from oz here on my setup (I saw your copy says it has been modified for validation, which also concerns me), and everything worked swimmingly with the slightly modified version of your gist. This is another reason I think having our own copy of the rng is going to be trouble - what are we changing about it anyway? Shouldn't validation against it 'just work'? And if not, is that a bug in the upstream version that we can send a pull request for?

Talking to @sloranz from the factory team, oz does the actual validation for them when they try to build something, and they will report back any such errors to Tim. I can still see value in 'pre-validating' the xml though, as a user may create a bunch of templates and not actually build them right off, so knowing they have any errors seems like a plus. This is a bit of a quandry to me. If you want to update the pull request for now with the newest rng file, that might be ok for this rev, but we need to revisit this. FWIW, tests still pass with the new file in place, so I am not sure what changes you needed from that old version

mtaylor commented 12 years ago

With regard to the differing RNG. This is actually to catch failures before build time. The reasoning is that the Oz tdl.rng has no restrictions on what operating system can be built. However, the implementation only provides functionality for a defined set of operating systems. This means even though the template description passes the validation, it fails to build. It was debated on where this extra data in the RNG should live a while back. As a temporary solution we added this information in conductors version. See: https://bugzilla.redhat.com/show_bug.cgi?id=795523

In order to prevent this bug reopening, I suggest we keep the current schema but add in the extra Mock operating systems values that we use for testing.

The reason the tests pass, with or without the updated RNG is because the template used in the tests uses a proper operating system defined in the above list.

I would like to see these changes introduced into a factory provided RNG, then we can completely forget about this whole thing.

Thanks for the review. I'll update the pull request accordingly.

mtaylor commented 12 years ago

The failure in Travis CI. Seems to be caused by a bug in rbx: https://github.com/rubinius/rubinius/issues/1813

jguiditta commented 12 years ago

For now, I am going to disable rubinius builds then, so we don't appear to have a broken build, when we really should not

jguiditta commented 12 years ago

Ok, as discussed this does work, though I think we'll want to revisit a few of the areas in this code at a later date. Thank you for making the changes that were feasible for this rev, pushing to master.