properly tagging storage option based on zcl.json #1140
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1140 +/- ##
==========================================
- Coverage 65.21% 65.20% -0.01%
==========================================
Files 170 170
Lines 18213 18214 +1
Branches 3922 3923 +1
==========================================
Hits 11877 11877
- Misses 6336 6337 +1
|
| @@ -560,6 +560,7 @@ function prepareCluster(cluster, context, isExtension = false) { | |||
| if (context.listsUseAttributeAccessInterface && attribute.$.entryType) { | |||
| storagePolicy = dbEnum.storagePolicy.attributeAccessInterface | |||
| } else if ( | |||
| context.listsUseAttributeAccessInterface && | |||
There was a problem hiding this comment.
Why this change? The attributeAccessInterfaceAttributes bit is not dependent on what lists do, is it?
There was a problem hiding this comment.
I actually thought this was a boolean that indicated whether or not the attributeAccessInterface was to be referenced or not but now I understand that attribute lists should ALSO be external. Will fix in a follow up. Thank you.
| staticAttribute.storagePolicy == | ||
| dbEnum.storagePolicy.attributeAccessInterface | ||
| ) { | ||
| staticAttribute.storagePolicy = dbEnum.storageOption.external |
There was a problem hiding this comment.
This is mixing up storage policy (which can be "any" or "attributeAccessInterface") and storage option, which can be "external", "ram", "nvm". See also #367
So what I would suggest is that instead of storing "storage option" values in a field that stores storage policy, instead we just use a local variable for the storage option to use for our db insert.
Incidentally, when I was looking over this setup, the resolve method on storagePolicy in db-enum.js is broken: it has storagePolicy.attributeAccess where it should have storagePolicy.attributeAccessInterface, no? Not directly relevant to this PR, but we should fix it here or in a followup.
There was a problem hiding this comment.
agree, I will make a follow up PR. Thank you.
|
@bzbarsky-apple it fixes this #1110 but in my opinion it was the first step in the natural progression of fixing all of the attribute storage issues. The front end needs to reflect reality before I can fix anything else. Will work on that one ASAP. |
|
also @bzbarsky-apple this issue #543 I have this one fixed locally but it requires me to change the XML files. Working on making a couple PR's. |
Saying that in the description, or at least linking the issue would have been helpful. No idea how someone is supposed to review a PR if it does not say what it's trying to do. |
#1110