Define a way how to undefine an attribute

[psss]

It would be useful to be able to undefine an inherited attribute. For example having an extra directory with helper files which should not be considered as tests. Perhaps something like this?

test: None

[psss]

Or shall we the special null value?
http://yaml.org/type/null.html

[sopos]

I faced it in the selinux [1] repo where there is attribute test defined in the root. I need to remove it for the beakerlib library.

I would not mess up with any reserved value. I would use the existing +, - notation but without a value. So

test-:

would mean to remove the attribute regardless the value(s).

1. https://src.fedoraproject.org/tests/selinux/

[psss]

So this would mean to use the special null value with the - suffix. Sounds ok. But I was thinking recently that we will need to reserve some prefix for special attributes anyway. So here's another option:

fmf_undefine: [test, path, duration]
fmf_forget: [test, path, duration]

This could be shorter than listing all attributes on separate line:

test-:
path-:
duration-:

And we could also have something like forget all inherited values:

fmf_undefine: all
fmf_forget: all

Finally, to give some more examples, why the prefix could be useful:

fmf_include: /some/fmf/identifier

fmf_include:
  - url: https://some.repo/
    name: /node/to/be/included

In this way we could reference (and graft) other fmf (sub)trees. What do you think?

[sopos]

Maybe instead of undefining we could also use replacing with an empty value test:, test-: would be special case for undefinning so fmf_undefine would work as well.

However I would prefer syntax closer to the already used. Both might be implemented though.

[psss]

Le'ts move this forward. Just ran into a use case where this would be super-needed: In the selinux tests namespace, there are tests in the root directory and thus it is not possible to store the test attribute at the top of the hierarchy and define a plan because it inherits the test attribute as well. @lukaszachy, would you agree to squeeze this into fmf-0.15?

[sopos]

but the test should be a leaf, not a node so I guess this should not happen. Like you need to create an extra leaf at the root to define a test, needn't you?

[martinky82]

I like the 'multiline' approach here. It is analogous to how the properties are (re)defined, thus IMHO it is slightly more legible for the user than a 'singleline' approach.

It might be

test-:
path-:
duration-:

or

-test:
-path:
-duration:

or, how about:

test: undef
path: undef
duration: undef

?

[psss]

but the test should be a leaf, not a node so I guess this should not happen

The selinux main.fmf in the root of the repo defines test. If we create let's say a /plans/basic.fmf it will inherit the test key and thus the node is considered to be a test. Actually it is both test and plan which is even more confusing.

[psss]

I like the 'multiline' approach here.

The problem I see with this is that it's not possible to undefine all attributes, e.g. starting from the scratch. Which would be very useful for the above-mentioned use case as well.

[martinky82]

test: undef
path: undef
duration: undef

and / or

undef: all ?

[sopos]

The selinux main.fmf in the root of the repo defines test. If we create let's say a /plans/basic.fmf it will inherit the test key and thus the node is considered to be a test. Actually it is both test and plan which is even more confusing.

I see. I think we talked about the possibility to explicitly set the object type. That would help in the contradictory cases. I could not find if there's a issue for that already, probably not.

If there's some keyword going to be used, there needs to be also a way how to use that keyword litelarly. I mean we need to define difference like this: to_be_removed: undef vs. no_to_be_removed: 'undef'.

[sopos]

I like the 'multiline' approach here.

The problem I see with this is that it's not possible to undefine all attributes, e.g. starting from the scratch. Which would be very useful for the above-mentioned use case as well.

If to_be_removed-: is ok for undefinning one attribute, what about *-: for undefinning all the attributes?
It still fits the concept of adding (x+: y) and removing (x-: y) individual values while quite self-explanatory as well.

[psss]

Looks like a nice smiley as well! :-) Pity that the * has a special meaning in yaml:

ERROR:

found undefined alias '-'
  in "<unicode string>", line 1, column 1:
    *-:
    ^

But it would be quite elegant ;-)

[sopos]

bloody yaml

[sopos]

it may be misleading but just -: is syntactically correct and still kind of makes sense

[pcahyna]

Hello, have you considered the existing Strategic merge patch format in Kubernetes that allow incremental updates of YAML/JSON data structures? https://stupefied-goodall-e282f7.netlify.app/contributors/devel/strategic-merge-patch/

I can provide some examples of how it would look like, if you provide example pairs of original and updated YAML.

[pcahyna]

To remove an attribute entirely, it would look like this:

test:
  $patch: delete

(Kubernetes strategic merge patch is an extension of JSON Merge Patch (http://erosb.github.io/post/json-patch-vs-merge-patch/), which allows to use test: null to express the same thing. But I would advise to not support this part of the standard, because it prevents using null as a special value outside of the context of updating something (missing test: and test: null would be forced to have the same semantics).)
Other Strategic merge patch directive options:
To update the dict entirely with new values:

test:
  $patch: replace
  foo: bar

To add a key to the dictionary, preserving the former keys:

test:
  $patch: merge
  foo: bar

I suspect that here replace would be the default to preserve the existing behavior and merge would need to be explicitly specified, if you want to implement it (this functionality is already there using the + sign, i believe).

[pcahyna]

I like the 'multiline' approach here.

The problem I see with this is that it's not possible to undefine all attributes, e.g. starting from the scratch. Which would be very useful for the above-mentioned use case as well.

If to_be_removed-: is ok for undefinning one attribute, what about *-: for undefinning all the attributes?

To undefine all the attributes, can't you simply replace the dict by an empty one?

i.e.

dict:
  foo: bar
  bar: baz

combined with just dict: {} gives dict: {}. The whole content of dict: gets replaced.

[pcahyna]

I like the 'multiline' approach here.

The problem I see with this is that it's not possible to undefine all attributes, e.g. starting from the scratch. Which would be very useful for the above-mentioned use case as well.

@psss I don't get it - I thought that unless + is used, redefining a dict means to start from scratch: https://fmf.readthedocs.io/en/latest/features.html#merging "When the + suffix is applied on dictionaries update() method is used to merge content of given dictionary instead of replacing it."

[lukaszachy]

@pcahyna Problem is that currently you can't remove attribute. All you can do in child nodes is to change the value, you can't delete the key.

Say my main.fmf in the root of the repo sets test attribute - All leaf nodes are treated as test because they have attribute test defined. It doesn't matter that its value is empty or None.

[lukaszachy]

Personally I like null as in test: null to remove the attribute.
IMHO if you need to remove more attributes you should consider changing the directory/repository structure.

[sopos]

Personally I like null as in test: null to remove the attribute.

Generally, the problem is that you may have the key without any value, meaning null, not an empty string.

IMHO if you need to remove more attributes you should consider changing the directory/repository structure.

Normally, in one repository, it is ok to have the tructure designed so it is not necessary to remove anything. But once you want to be able to load some sub-structure then it might be worth it.

[pcahyna]

@pcahyna Problem is that currently you can't remove attribute. All you can do in child nodes is to change the value, you can't delete the key.

I understand this point - what was it a reply to?

[pcahyna]

Say my main.fmf in the root of the repo sets test attribute - All leaf nodes are treated as test because they have attribute test defined. It doesn't matter that its value is empty or None.

Personally I like null as in test: null to remove the attribute.

As @sopos pointed out, empty value is actually translated to null. So test: null is the same as just test:.

[lukaszachy]

Generally, the problem is that you may have the key without any value, meaning null, not an empty string.

Sure, but the current specification doesn't require it and frankly, if there is ever a need to assign any semantic to the key presence you can do the same outcome with boolean. So I think about that just as a hypothetical problem.

@pcahyna

As @sopos pointed out, empty value is actually translated to null. So est: null is the same as just test:.

For me test: null is less confusing. So let's say that if yaml assigns null value to the attribute then the attribute should be removed from the node (and for its children)

[pcahyna]

As @sopos pointed out, empty value is actually translated to null. So est: null is the same as just test:.

For me test: null is less confusing. So let's say that if yaml assigns null value to the attribute then the attribute should be removed from the node (and for its children)

@lukaszachy, you said

Say my main.fmf in the root of the repo sets test attribute - All leaf nodes are treated as test because they have attribute test defined. It doesn't matter that its value is empty or None.

but if you specify that test: null is equivalent to test not being defined at all, leaf nodes where test have a value of empty or None (assuming that you meant null) will not be treated as test anymore. So this is in principle a change in behavior. (I don't claim that it is necessarily a serious one, but it is a change nevertheless.)

[psss]

The proposed solution for undefining a single key using key: null or key: is nicely concise. But what I'm now sure about is that this approach completely removes the possibility to intentionally set an attribute to null or None which might be useful / needed. Here's a recent example from tmt where the integration test does not belong into any tier. Here it's possible to use tier: [] but there could cases where null will be more necessary.

Thus I would propose to choose a syntax which does not limit values or key naming. Except for + and - suffixes there's only one more reserved character, / which is used for introducing the virtual hierarchy. If used without a child name as /: it could serve for special purposes like this, without introducing any limitation. Here's some brainstorm:

Do not inherit from parent:

/:

Undefine selected keys:

/:
    remove: [test]

Graft a local branch:

/:
    graft: /some/local/node

Graft a remote tree:

/:
    graft:
        url: https://some.repo/
        ref: devel

For possible operation names we can get some inspiration from links provided by @pcahyna:

• http://erosb.github.io/post/json-patch-vs-merge-patch/
• https://tools.ietf.org/html/rfc6902
• https://tools.ietf.org/html/rfc7386
• https://linux-system-roles.github.io/documentation/incremental_settings.html
• https://stupefied-goodall-e282f7.netlify.app/contributors/devel/strategic-merge-patch/

Thoughts?

[lukaszachy]

I like this proposal even though /: can already be used to create also a virtual node without suffix.
Will /: be the first thing to do? So one can resolve parent's inheritance first and then define virtual nodes in the same file. E.g.

test: foo
/:
/child1:
  attr: value

[sopos]

Personally I do not like much just /:.

Currently I'm not sure if there is a consensus on removing individual attribures.
However if my proposal of to_be_deleted-: will be the winning one, and it pretty much fits the current behavior. What about /-:? That would still follow the syntax principle that - suffix means to remove something with the addition if the value is empty it means to remove everything.

As @lukaszachy noted /: currently refers to an unnamed object one level deeper but it is still less misleading than pure -: which, on the other hand, would refer exactly to the current object.

[pcahyna]

I think you should first agree on the semantics before discussing the syntax. There are two possible semantics for deleting something:

• Set the key itself to some special value that means "this should be deleted". Using null or some special dict like the Kubernetes strategic merge patch $patch: delete falls in this category, as apparently does @psss' /: to stop inheriting. ("Stop inheriting" means "remove everything", right?)
• Use a special directive on the parent that means "the named child should be deleted". to_be_deleted-: and

/:
    remove: [test]

fall in this category. (Kubernetes strategic merge patch has $deleteFromPrimitiveList that also works this way, I haven't mentioned it so far, because I did not want to complicate the issue further.)
The advantage of the second approach is its symmetry with the current + suffix.

I have a question about the intended semantics of -. If I have a dict like this

dict:
  a: x
  b: y
  c: z

I would be able to use dict-: to remove the entire dict and dict-: with attributes to remove only parts of dict? Applying

dict-:
   b:
   c:

to the dict above would result in dict being just

dict:
  a: x

correct?
Would it be equivalent to

dict:
  b-:
  c-:

? Hmm, perhaps not, because dict: without + just replaces the original dict, right?

[sopos]

Not sure about handling inside dict attributes, but list items work.

list:
- a
- b

list-:
- a

results in

list:
- b

[martinky82]

How about (again) using special keyword undef? It is short, simple and very intuitive - being used for exactly that in several programming languages. It may be used either as:

undef: x,y, z

or

x: undef
y: undef
z: undef

Being a keyword, if anyone needs undef as a value, it's enough to wrap it in poarentheses:

a: 'undef'

and it does not block the user from having Null as a value.

The x-: approch is nice, but I'm afraid the - sign will be just too easy to overlook.

[pcahyna]

How about (again) using special keyword undef? It is short, simple and very intuitive - being used for exactly that in several programming languages. It may be used either as:

undef: x,y, z

or

x: undef
y: undef
z: undef

Being a keyword, if anyone needs undef as a value, it's enough to wrap it in poarentheses:

a: 'undef'

I thought that fmf was a layer above YAML... but your proposal is incompatible with YAML, because in YAML a: 'undef' and a: undef are exactly equivalent.

[sopos]

The x-: approch is nice, but I'm afraid the - sign will be just too easy to overlook.

I'm just trying to emphasize the fact that - suffix approach is there for some time already and I think it is good idea to be consistent - doing similar thing similarly, thus removing / undeffinig using that - suffix.

Then, the remaining questions would be "how to wipe a full list, e.g. require", and "how to do a full cleanup (all attributes at the level)".

Moreover, I can imagine a request to be able to remove all but some specific attribute(s). This is something which may be a problem with the - suffix approach.

[pcahyna]

Then, the remaining questions would be "how to wipe a full list, e.g. require", and "how to do a full cleanup (all attributes at the level)".

I am probably missing something, but I thought that unless you add the + suffix, nothing is inherited, i.e. lists get fully wiped, all attributes are cleaned (and everything is replaced by the new values that you are supplying).

[sopos]

I am probably missing something, but I thought that unless you add the + suffix, nothing is inherited, i.e. lists get fully wiped, all attributes are cleaned (and everything is replaced by the new values that you are supplying).

By default, everything is inherited and you can replace attributes by defining them again or you can modify them (lists) by the - or + suffixes. But in case of the - suffix you need to specify which particular item of the list you want to remove.

The list described in #14 (comment) may come from the parent fmf node.

$ cat main.fmf 
list:
- a
- b

/level1:
  list+:
  - c
  list-:
  - a

/level1a:
  list:

/level1b:
  list2:

/level1c:
  list:
  - d

$ fmf show
/level1
list: b and c

/level1a
list: None

/level1b
list: a and b
list2: None

/level1c
list: d

Note that a need for presence of the list2 in /level1b is probably a bug.

[pcahyna]

@sopos thanks, so what I was missing was that nodes are treated differently from other dicts. Nodes behave (in terms of the strategic merge patch standard) as if $patch: merge were the default, while other dicts behave as if $patch: replace were the default. For other (non-node) dicts you can switch to an equivalent of $patch: merge by appending + to the key name that references the dict, but there is no way to specify $patch: replace on nodes.

[sopos]

@sopos thanks, so what I was missing was that nodes are treated differently from other dicts. Nodes behave (in terms of the strategic merge patch standard) as if $patch: merge were the default, while other dicts behave as if $patch: replace were the default. For other (non-node) dicts you can switch to an equivalent of $patch: merge by appending + to the key name that references the dict, but there is no way to specify $patch: replace on nodes.

Right, the $patch: replace for a node is what I call a full cleanup.

[pcahyna]

@sopos

I'm just trying to emphasize the fact that - suffix approach is there for some time already and I think it is good idea to be consistent - doing similar thing similarly, thus removing / undeffinig using that - suffix.

the problem is that currently - works by listing individual attributes/list elements that you want to remove. You add - to the parent of the items that you want to remove. But in your proposal, adding it to the item would mean to remove the item itself (if an empty value is given). This would be a special case, inconsistent with the current usage.
Do I understand correctly that

dict-:
   b:
   c:

would be equivalent to

dict:
  b-:
  c-:

?
If so, why do you need a special new case at all, can't you simply use the existing semantics of -?
I.e. to remove test, instead of writing test-: as you propose, you would simply write

/parent-:
  test:

assuming that the test attribute that you need to remove is under a node called /parent. I suppose that the problem might be that the parent node is often implicit, i.e. does not exist as a dict in YAML?

Then, the remaining questions would be "how to wipe a full list, e.g. require",

You can simply set the list to an empty list, no? require: []

and "how to do a full cleanup (all attributes at the level)".

If you don't want to introduce a new special attribute like the Kubernetes strategic merge patch's $previous and you like operator-like suffixes, you can introduce a = suffix, analogous to the + and - suffixes.

[lukaszachy]

Another use case this should cover: repository with tests, in root's main.fmf are contact and component attributes (common to all tests). However I'd like to have also plan (for CI of tests) defined - currently there is no way where to place plan.fmf so that root's main.fmf is ignored and thus each plan has contact/component attributes which make tmt plan lint to fail.

Note that if I have to choose between failing tmt plans lint and removing root's main.fmf (or moving all tests to subdir where main.fmf can live) I pick screaming linter.

So ideally I'd have way how to undefine attribute in plan.fmf or mark plan.fmf to stop inheriting from its parents (in this case root main.fmf)

[sopos]

definitely +1 for a possibility to cut off the inheritance completely - sometimes it is simply necessary

[jscotka]

I've suggested as side effect to use some special mark in my PR: #159
What defines ! mark as last element occurence.
This works well and it is clear. Just bad is that you have to remove inheritace in parent node, but theoretically it make sense as well, just addind some predecessor

so example maye explain it:

test: runtest.sh
/test1:
   name: this is test
/plans:
    test!: None (This is not important what will be there)
    /tier1:
       summary; This is plan tier1

so it is clear how to do it, just you have to add some one more layer, before your definition with undefined node. what is probably fine. it is not so often case to need to undefine some values.

and theoretically I can imagine also some syntax, what will remove all items from inheritance, something like:

test: runtest.sh
/plans!:
   /tier1:
     summary: this is tier1

so that /plan! causes last occurence of metadata in this node, and do not pass them to childrens. This may be little bit nonintuitive, as someone may expect that items are removed on this level, not in childrens, but could be described well.

Conclusion

Why this idea is clean is: Syntax is that ! means last occurence, so that for removing in all childs, you just add this last occurence layer before your definition.

[lukaszachy]

After some time I'd say I'm voting for /: approach

[jscotka]

I like my idea :-) with ! but after some reconsideration, I think that the approach with /: is not bad, but I think that there is not reason to add semantics directly to fmf files for this purpose, what is hidden s that harder to show this , better to use .fmf/ dir and there e.g. put some config like git configs .fmf/remove: containing e.g. list of files or regexp to node where stop inheritance.

[lukaszachy]

... but I think that there is not reason to add semantics directly to fmf files for this purpose, what is hidden s that harder to show this , better to use .fmf/ dir and there e.g. put some config like git configs .fmf/remove: containing e.g. list of files or regexp to node where stop inheritance.

I think that is more confusing to the user and one would need to be more careful when copying fmf files between roots (hint: need to remember to check .fmf dir for configuration)

If I want to cut inheritance in the node I want to express it in that node itself.

[jscotka]

... but I think that there is not reason to add semantics directly to fmf files for this purpose, what is hidden s that harder to show this , better to use .fmf/ dir and there e.g. put some config like git configs .fmf/remove: containing e.g. list of files or regexp to node where stop inheritance.

I think that is more confusing to the user and one would need to be more careful when copying fmf files between roots (hint: need to remember to check .fmf dir for configuration)

If I want to cut inheritance in the node I want to express it in that node itself.

I understand, but this is exactly users often does, copy .fmf dir and do not call fmf init, and there are not useful info. I can imagine it will check the info there e.g. current git repo and in case it does not match, raise WARN to user, to avoid this behaviour, as in fiuture there could be more info and this copying is bad idea.

and in your cases I understand that this info is close to node, but when I get some fmf data of somebody and I do not know about that it is hard to understand why it behaves in another way than I expect. and it is simplier to do cat .fmf/remove than doing some grep -R -a 5 '\/:' . especially tmt probably will con contain any tooling for this and nobody currenlty use pure fmf command to debug some data inside FMF files

[psss]

After some time I'd say I'm voting for /: approach

Agreed. This currently seems to be the best way. By using this special case of empty node name for fmf directives we get rid of confusing nodes ending with a / plus this gives us a good extensibility for the future as we can define more keywords under it. Implementation of the first directive inherit with the most requested use case ready for review in #170.