Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add directive make-leaf to force selecting node #223

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lukaszachy
Copy link
Collaborator

@lukaszachy lukaszachy commented Feb 27, 2024

Q: Should we bump the fmf version to '2'? If one uses this directive with older fmf, the ERROR Unknown fmf directive 'is-leaf' ... happens.

  • implement the feature
  • write the documentation
  • extend the test coverage

@lukaszachy lukaszachy added this to the 1.4 milestone Feb 27, 2024
@lukaszachy
Copy link
Collaborator Author

Limiting tests in #225. Lets see

Expected usage with virtual tests while keeping the parent
test as well

Fix: #221
@jscotka
Copy link
Collaborator

jscotka commented Apr 16, 2024

Hi, I would love some more descriptive name, instead of is-leaf e.g. someghing like marked-as_leaf, or I can imagine something underscoder, to avoid confusion user data and TMT will then ignore validation these attributes, like:
_mark_as_leaf: True and in TMT there could be rule to skip underscored items, as they are directives and should not be used as metadata source directly. But be backward compatible, and display it to users as well

@happz
Copy link
Collaborator

happz commented Apr 16, 2024

... or I can imagine something underscoder, to avoid confusion user data and TMT will then ignore validation these attributes, like: _mark_as_leaf: True and in TMT there could be rule to skip underscored items

tmt can easily ignore directives with dashes simply by listing them in tmt sources, if needed, plus users may define their custom keys with underscores in their tests (extra-foo_bar, https://github.com/teemtee/tmt/blob/main/tmt/schemas/test.yaml#L128), therefore a rule as general as "ignore everything with underscores" might cause more harm than good.

I suspect these directives don't even reach tmt, do they? They are consumed by fmf, applied and tmt gets the final materialized tree of data, correct?

@LecrisUT
Copy link

What about also having the opposite? I.e. marking a file.fmf as non-leaf. I was thinking if this might be necessary in the context of:

fmf/fmf/base.py

Lines 812 to 815 in 29190c7

Example usage:
with Tree('.').find('/tests/core/smoke') as test:
test['tier'] = 0


More specifically, in LecrisUT/tmt-copr#2 I want discover plugin to inject main.fmf-like file into:

/prefix:
  framework: copr
  test: custom-placeholder
/prefix/pkg1:
  package: pkg1

Then there could be overrides defined in /prefix/pkg1.fmf like

result: xfail

The idea of adding a function to mark something as non-leaf is if we only have a /prefix.fmf file:

duration: 10h

In this case tmt lint will fail because /prefix.fmf is neither a plan, nor a test. It would be nice to mark this as being non-leaf so that it is not parsed by tmt lint (or at least not before discover is run) or other phases.

@lukaszachy
Copy link
Collaborator Author

I suspect these directives don't even reach tmt, do they? They are consumed by fmf, applied and tmt gets the final materialized tree of data, correct?

Correct.

@lukaszachy
Copy link
Collaborator Author

What about also having the opposite?

I see no problem - mark-non-leaf as the name for the directive? If both are set it will result with fmf.utils.FormatError

@jscotka
Copy link
Collaborator

jscotka commented Apr 16, 2024

I suspect these directives don't even reach tmt, do they? They are consumed by fmf, applied and tmt gets the final materialized tree of data, correct?

Correct.

This is question. On first singht, I can imagine that TMT will interpret the value, but another is, that it is semantic of FMF, so that it should be materialized and TMT will do not see this key anyway.
as @happz mentioned, the underscored semantics could cause harm in case of ignoring them. In opposite side, when someone using is-leaf attribute today. it will break someone workflow in future, so that Yes, This underscoring is not good, But I would like to see also some special mark to intepret these values, e.g. long time ago, we've discussed of using ./: or something similar as node name path.
so that I can imagine also sometghing like:

this_is_leaf/:
  ./:
    mark_as_leaf: True
    another_fmf_firective: something
  test: shell.sh

BTW, this disussion is closer to FMF/TMT config/profiles (#196) than we should allow :-), it allows you to configure/override default FMF behaviour, what do you think? From one perspective it is very simple PR, from another perspective overriding, redefining nodes or change some behaviour seems to me very close to profiles and configs, that's Why I would like to see something more generic, than just one attribute to change befaviour of FMF, but do it more extensible.

@lukaszachy
Copy link
Collaborator Author

This is question. On first singht, I can imagine that TMT will interpret the value, but another is, that it is semantic of FMF, so that it should be materialized and TMT will do not see this key anyway.

tmt doesn't see this value, similar to merge suffixes (+, -, +<), all of this is processed by fmf.

@lukaszachy
Copy link
Collaborator Author

BTW, this disussion is closer to FMF/TMT config/profiles (#196) than we should allow :-), it allows you to configure/override default FMF behaviour,

I beg do differ - for me this is just setting fmf directive. By overriding default fmf behavour I'd expect custom _merge_special, injection of directives not known to fmf or similar.

@lukaszachy
Copy link
Collaborator Author

Hm, I'll leave the mark-non-leaf feature to the future. As it changes the flow of creating the tree a lot -- if I understood correctly such nodes will be effectively 'main.fmf' -> not sure what to do if more are present, which data win...

@LecrisUT
Copy link

not sure what to do if more are present

Not sure what you mean here. It would have the same inheritance rules, i.e. anything within that file is marked as non-leaf. I was thinking of just adding a special case of Tree.children=dict(None: None) or something similar, and guard inside the generators to not export that, e.g.:

fmf/fmf/base.py

Lines 573 to 579 in 29190c7

def climb(self, whole=False):
""" Climb through the tree (iterate leaf/all nodes) """
if whole or not self.children:
yield self
for name, child in self.children.items():
for node in child.climb(whole):
yield node

@lukaszachy
Copy link
Collaborator Author

Could you please create a new issue, @LecrisUT ?

What I have in mind is this:

.
├── bar
│   └── main.fmf
├── .fmf
│   └── version
├── foo.fmf
└── main

Currently this produces leaf /, non-leaves /foo and /bar. Inheritance works in way that /bar is created based on 'main.fmf' and 'bar/main.fmf'.

If foo.fmf is 'mark-non-leaf' - how it will ever become a leaf aka node usable by tmt?
Should it become parent of some none, similar to the 'main.fmf'?

@lukaszachy lukaszachy changed the title Add directive is-leaf to force selecting node Add directive mark-leaf to force selecting node Apr 16, 2024
@lukaszachy lukaszachy changed the title Add directive mark-leaf to force selecting node Add directive make-leaf to force selecting node Apr 16, 2024
@LecrisUT
Copy link

LecrisUT commented Apr 16, 2024

Currently this produces leaf /, non-leaves /foo and /bar.

You mean the opposite right? non-leaf / and /foo, /bar are leaves (in the current implementation, and discussion does not touch the current implementation right?)

If foo.fmf is 'mark-non-leaf' - how it will ever become a leaf aka node usable by tmt?
Should it become parent of some none, similar to the 'main.fmf'?

The idea is to support the inverse of grafting where the base is populated by discover and on top of that we apply fmf.Tree from filesystem:

  • grafting: have a pre-defined fmf.Tree from filesystem then add on top of that another fmf.Tree (from dict usually)
  • inverse-grafting: construct an fmf.Tree-like structure (/ in the example), then apply fmf.Tree from filesystem (foo.fmf file in this example)

But I guess in both cases you could have breaking tmt lint due to incomplete data, so maybe it only needs to be on the tmt side (other than graft/inverse_graft support on fmf side)

This was referenced Apr 16, 2024
@lukaszachy
Copy link
Collaborator Author

You mean the opposite right? non-leaf / and /foo, /bar are leaves (in the current implementation, and discussion does not touch the current implementation right?)

Right you are, I flipped those two terms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keeping the 'node' as an object even though it has children and is not a leaf
4 participants