Code review comment for lp:~vladimir.p/nova/vsa

Revision history for this message
Soren Hansen (soren) wrote :

> Nicely done vladimir. I just see a couple of issues:
>
> 573 === added directory 'nova/CA/newcerts'
> 574 === removed directory 'nova/CA/newcerts'
> 575 === added file 'nova/CA/newcerts/.placeholder'
> 576 === removed file 'nova/CA/newcerts/.placeholder'
> 577 === added directory 'nova/CA/private'
> 578 === removed directory 'nova/CA/private'
> 579 === added file 'nova/CA/private/.placeholder'
> 580 === removed file 'nova/CA/private/.placeholder'
> 581 === added directory 'nova/CA/projects'
> 582 === removed directory 'nova/CA/projects'
> 583 === added file 'nova/CA/projects/.gitignore'
[...]
> Some weird stuff going here. Might be nice to recreate the branch to get rid
> of these strange issues. i.e. start with trunk and just copy all of the new
> files in. and put it in one commit.

No need. Just do something like:
bzr revert -r -150 nova/CA/newcerts/.placeholder nova/CA/private/.placeholder nova/CA/projects/.*

-150 picked at random. It just needed to be a revision from before they got removed.

bzr assigns a file id to a file when you create it. It you rename it, it keeps this file id. If you remove a file and later create another one with the same name, bzr considers it a new file and assigns a new file id to it. If you don't know this, you run into annoying, frustrating stuff like this. However, it lets you e.g. rename a file from "bar" to "baz" and rename another from "foo" to "bar" and bzr actually knows that this is what you did instead of thinking you deleted "foo", created a brand new file "baz", and made some really funky changes to "bar". :)

bzr revert resurrects those files (with their ids) from the given revision.

> In general I'm worried about adding something to the project that is specific
> to a vendor, but since you guys have put so much effort into doing this the
> right way, I'm willing to assume that you will continue to generalize and make
> it possible for others to use this code.

I understand where you're coming from with this concern. However, this seems like a really good driver that others can expand on.

> Rest of nova-core, does that seem ok?

I'm ok with it. I don't see us gaining anything by having this live outside of Nova itself. And, I like the code and would love to see others doing similar stuff building on top of this rather than building something crappy themselves.

Approved (assuming you do the quick bzr thing above).

review: Approve

« Back to merge proposal