-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support inheritance when parent is a list and child is not a list #228
Comments
Supporting this use case sounds like a reasonable extension of the current implementation. Might be a little bit confusing to users but, I guess, if it is well documented, should be ok. To sum up the proposed approach when parent is a list:
Any concerns about this approach? I guess, similar should be done for the |
+1 for this, should be very useful |
I think this should be better handled in |
@LecrisUT Is that a necessary complication? What you mean by "Currently this can be circumvented by making the discover+: be a list."? e.g. have two phases and add new attribute to each of them |
The following should work (note the extra discover:
- how: fmf
url: https://github.com/project1
- how: fmf
url: https://github.com/project2
/basic:
summary: Quick set of basic functionality tests
discover+:
- filter: tier:1 $ fmf show
/test/basic
discover: [{'how': 'fmf', 'url': 'https://github.com/project1'},
{'how': 'fmf', 'url': 'https://github.com/project2'},
{'filter': 'tier:1'}]
summary: Quick set of basic functionality tests
Yes, I want to use This change is currently very |
this doesn't work. It doesn't update the current entries, but add a new entry to the list {how: shell, filter: 'tier:1'} |
Aah, I misunderstood the situation. You mean the What about using a more yaml-like syntax like |
I personally don't have any syntax preference it is just currently afaik there is no way I can add/update items when it is in a list. |
Whole merging stuff is very error prone already. If time allows there is a plan to provide command to 'explain what is being done with data...' which could put some light into it if one is in bind. Currently we have traceback when merging list with dict (or any other type) - Thus any change in this sense will not break an existing valid fmf files. The But having way to customize merge_special might be handy for others, that is for sure. Could you file an RFE for that? |
I'm here with @lukaszachy. The current behavior is not changed, only the invalid combinations (e.g. list+non-list which are now tracebacking) would be interpreted in a different way. So this would be just extending the current implementation. On the other hand, catching exceptions for unsupported operations can be also a valid use case (not for So, perhaps, a dedicated operator for this could be a little bit better, to make it obvious at the first sight that something different than just a regular addition is happening? Sounds more in the direction of |
Actually, I would be fine with using the same operator, in this case :) I might be getting it wrong, but IIUIC, the goal is to enable the It seems similar to |
@happz, actually, the proposed implementation is different, see the comment above for details. Basically it aims to cover the use case provided by @bgoncalv, for example adding the same |
I would also appreciate having this implemented, it is very useful for plans that are more or less the same but only differ slightly by the test selection (we have a very clear and specific use case for CI). I just want to double check that I understand it correctly. Assume I have the following plans:
Do I understand correctly that selection in plans A, B, C is applied on all tests found by the main discover? IOW
Is all this compatible with the original request and proposed approach? |
The reason why this could be pretty useful is that in general you have expect many items in discover phase (mutliple test sources, some of them using filters, some of them using explicit test selection, etc.) and you might easily end up with a discover phase that have ~100 lines of code. Now you want to run the plan in different configurations, e.g. you need to run the same plan with and without buildroot - obviously you want to add another filter for that but other than that the discover is pretty much the same regardless of buildroot. Add testing in FIPS and from 2 plans you have 4 - again, the only difference between discover phases in all 4 plans is that you want to use a different test selection. With 4 plans we are already talking about significant duplication of discover phases (unless this RFE is implemented). |
If you have a list like: /B:
discover+:
- filter: tier:2 Then the current implementation applies and you get: /B:
discover:
- how: fmf
url: https://github.com/project1
- how: fmf
url: https://github.com/project2
filter: tier:1
- how: fmf
url: https://github.com/project3
tests:
- first # this one has tier: 1
- second # this one has tier: 2
- third # this one has tier: 3
- filter: tier:2 The proposal here kicks in when you make the value a dict (or string) /B:
discover+:
filter: tier:2 Then it would be like the /B:
discover:
- how: fmf
url: https://github.com/project1
filter: tier:2
- how: fmf
url: https://github.com/project2
filter: tier:2
- how: fmf
url: https://github.com/project3
tests:
- first # this one has tier: 1
- second # this one has tier: 2
- third # this one has tier: 3
filter: tier:2 |
Yes, so it is basically what I want. I assume that it is already (in the current 'inheritance' implementation) handled that if you have, for instance, a filter already present in the main discover phase and you are adding one more in discover+ they are merged into the list (ie. final result is a conjunction of both) and this applies not just to filter but e.g. to tests too (except that the result is an intersection). |
There is no special treatment for So you can still add |
Just to clarify the discover:
how: fmf
filter:
- tier:1
/extend:
discover+:
filter+:
- tier:2
/override:
discover+:
filter:
- tier:2 You would get this in the data:
For the multiple-phase-update case proposed here it should behave similarly. Just note, that the plan is to use a dedicated operator in order to prevent confusion with the normal merging using the Btw, any idea about a suitable character to be used for this? Trying some brainstorm:
Any better ideas? |
Might create more confusion due to lack of support for other operators, but how about |
We have a similar need to teemtee/tmt#3198 we need the inverse of this, we have common arguments for discover we want each discover list item to inherit main.fmf discover:
how: fmf
prune: True
adjust-tests:
- duration+: '*3'
when: hw == slow plan.fmf discover+:
- name: upstream
url: https://some.url
- name: downstream
url: https://other.url I guess the solution will be same/similar for both versions of this issue, so not creating another one. |
Looking at https://tmt.readthedocs.io/en/stable/guide.html#inherit-plans
there are 2 plans that extend the discover with
filter
option. This works well, as discover is a dict.discover
can also be a list, but in that case the inheritance doesn't work.It causes error:
MergeError: Key 'discover' in /basic (can only concatenate list (not "dict") to list).
My assumption would be that each entry in the list would be updated with provided dictionary.
The text was updated successfully, but these errors were encountered: