BTW, in node.py:1318 is "node" hardcoded again (next to a couple of other hardcoded values).
But as I was looking over it, I came to the same conclusion as you: there is no need for dynamic and static fields, and there is not reason why SF_NODE should not be in VALID_STORAGE_FIELDS. Cheers, Thomas On Thu, Aug 22, 2013 at 12:11 AM, <[email protected]> wrote: > Question: does 'SF_NODE' belong in 'VALID_STORAGE_FIELDS' ? > > > Let's take a look at two files referring to 'VALID_STORAGE_FIELDS' and > 'SF_NODE' and some code snippets (I omitted some irrelevant files). > > > /lib/cmdlib/node.py: > > Someone knows '_CheckOutputFields' well can perhaps enlighten why > 'SF_NODE' is being passed as a static field instead of a dynamic > field. I looked inside '_CheckOutputFields' and static and dynamic > fields are merged together and compared against selected. I don't > see a reason why they should be passed separate. And > '_FIELDS_STATIC' is not used anywhere else and it is not part of any > of the superclasses. I put the code here so you can see for > yourselves. > > _FIELDS_STATIC = utils.FieldSet(constants.SF_NODE) > > ... > > def CheckArguments(self): > _CheckOutputFields(static=self._FIELDS_STATIC, > > dynamic=utils.FieldSet(*constants.VALID_STORAGE_FIELDS), > selected=self.op.output_fields) > > > ./qa/qa_node.py > > So this one is just inconsistent: you can see that we are creating a > list containing 'VALID_STORAGE_FIELDS' plus 'SF_NODE' and 'SF_TYPE'. > This suggests these two last contants should not be a part of > 'VALID_STORAGE_TYPES'. However, 'SF_TYPE' IS part of the > 'VALID_STORAGE_FIELDS' and 'SF_NODE' is NOT, so the final list is > going to contain 'SF_TYPE' twice. > > cmd = ["gnt-node", "list-storage", "--storage-type", storage_type, > "--output=%s" % ",".join(list(constants.VALID_STORAGE_FIELDS) + > [constants.SF_NODE, > constants.SF_TYPE])] > > > So at this point, we can either (1) fix the QA node test to by > removing 'SF_TYPE' from the list; (2) or we can add 'SF_NODE' to > 'VALID_STORAGE_TYPES', fix the LU by eliminating the '_STATIC_FIELDS', > and removing both 'SF_NODE' and 'SF_TYPE' from the QA node test. > > If we go with number 2, we can also simplify '_CheckOutputArguments' > because it is only used in two places: in this one and another one > that is very similar. And we can stop this thing with 'static' and > 'dynamic' fields and simply call it 'fields'. > > Do you guys see any other problem I might have overlooked ? > Whay do you think ? > > Jose > > On Wed, Aug 21, 2013 at 02:54:44PM +0200, Guido Trotter wrote: > > On Tue, Aug 20, 2013 at 9:24 PM, <[email protected]> wrote: > > > Hi, > > > > > > In lib/constants.py, is 'VALID_STORAGE_FIELDS' missing 'SF_NODE' ? > > > > > > > Possibly, indeed. Can you investigate where it's used and report on > > whether it should indeed be added, or just dropped, perhaps? > > > > Thanks, > > > > Guido > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
