So, I ended up taking another more thorough look at the helper commands and gnt-* man pages. There was a lot of inconsistent syntax and I tried to clean it up and reflect what the commands actually do with regards to UUID vs resource name.
Now this patch has gotten a bit bigger than I originally planned it to be, but it's only cosmetic changes and documentation, no functional change. On 4 January 2017 at 12:18, 'Brian Foley' via ganeti-devel <[email protected]> wrote: > On Tue, Jan 03, 2017 at 09:35:20AM +0000, Federico Pareschi wrote: >> Interesting, you raise valid points for a patch that I thought was >> trivial but indeed is not. >> >> > Per our discussion yesterday, all the gnt-group commands actually do work >> > with >> > either group specified by name or by UUID, so this is an improvement to the >> > docs. However, the command line help in lib/client/gnt_group.py also uses >> > the >> > string "group-name". Could you update these too please? >> >> >> Yes, good point. >> >> > Similarly, could you make sure you update all the other lib/client/*.py >> > CLI arg >> > strings to match the changes you've made? >> >> >> Most of the other commands, surprisingly, were already correct in >> their .py files, >> however I have fixed the few that were not. > > Great. Thanks. > >> > Interestingly, gnt-instance shutdown/startup work correctly with either >> > UUIDs, >> > or prefix matches on instance FQDN. However, strangely, some of the >> > subcommands >> > like gnt-instance info will do prefix match on instance FQDN, but don't >> > work >> > with UUID, so I think we can't just bindly change these without checking >> > what >> > each subcommand actually uses. >> >> >> This looks like a symptom of something broken/inconsistent that should >> get fixed. >> First of all, the changes I had made were already in the .py helper >> strings, interestingly enough. i.e. gnt-instance reboot --help already >> shows <instance> and not <name> so I think that's a good change as is >> (and indeed it does work with UUIDs as well). >> >> The real problem, though, is that as you said some of these commands >> are just inconsistent. For example all the instance tagging commands >> do not work with UUIDs, however the group tagging does work with UUIDs >> and they all use the same base code. This leads me to believe there is >> some inconsistency in the backend code for tagging objects through >> UUIDs vs the object name and it should get fixed, but it's probably >> low priority and outside of the scope of this patch. >> >> Now the question is, do we just avoid touching all the tags-related >> info strings and fix the rest of the man pages as proposed in this >> patch, or do we change the tags-string for group (but not for >> instance/node objects) leaving them inconsistent in the documentation >> (but consistent in the way the code behaves)? > > Good question. Before I went on holidays I tried to figure out where exactly > the arg parsing/'name resolution' was being done, and as you say it seems > inconsistent. > > My inclination would be to say that it's better for the docs to be accurate > and > expose the inconsistency in the implementation, than to have nice 'regular' > docs that aren't accurate. > > If we intend to fix the inconsistent implementation in the near future, I'd > say > leave the docs alone, and just fix the implementation; but if we don't want to > go down the rabbit-hole of fixing the impl, then just update the docs. > > [snip] > >> > gnt-node list-drbd, list-tags definitely looks up by prefix match on FQDN >> > or >> > UUID. Is this definitely true for gnt-node add, and the others though? >> >> I tried one example, gnt-node remove <UUID> and indeed it does not >> work with UUIDs. This really bothers me as leaving it as it is, with >> node_name, makes it inconsistent with a lot of the rest of the code >> where UUIDs do work. Again, we should be fixing this but it's probably >> very low priority compared to other issues. > > Agreed. It's pretty low priority, and I don't recall anyone else complaining > about it. > >> I'll revert the changes in those flags that do not use UUIDs. I'll >> send a new patch after we've decided what we should do with the >> tags-related code as I mentioned earlier. > > Thanks, > Brian.
