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

Both plan and execute keys should not be allowed #1648

Open
psss opened this issue Oct 27, 2022 · 5 comments
Open

Both plan and execute keys should not be allowed #1648

psss opened this issue Oct 27, 2022 · 5 comments

Comments

@psss
Copy link
Collaborator

psss commented Oct 27, 2022

When playing with #1646 I realized that if both plan and execute keys are present in an fmf node tmt gives only a warning. For the following plan:

plan:
    import:
        url: https://github.com/teemtee/tests

execute:
    how: tmt

The output looks like this:

tmt plan ls
warn: /plans/remote/tests: - {'adjust': [{'prepare+': [{'name': 'tmt', 'how': 'install', 'directory': 'tmp/RPMS/noarch'}], 'when': 'how == full'}], 'execute': {'how': 'tmt'}, 'provision': {'how': 'local'}, 'plan': {'import': {'url': 'https://github.com/teemtee/tests', 'name': '/plans/polarion'}}} is valid under each of ordereddict([('required', ['plan'])]), ordereddict([('required', ['execute'])])
warn: /plans/remote/tests: - {'adjust': [{'prepare+': [{'name': 'tmt', 'how': 'install', 'directory': 'tmp/RPMS/noarch'}], 'when': 'how == full'}], 'execute': {'how': 'tmt', 'name': 'default-0'}, 'provision': {'how': 'local', 'name': 'default-0'}, 'plan': {'import': {'url': 'https://github.com/teemtee/tests', 'name': '/plans/polarion'}}} is valid under each of ordereddict([('required', ['plan'])]), ordereddict([('required', ['execute'])])
/plans/remote/tests
/plans/remote/tests

Note that the plan is listed twice as it is detected using both keys. I see two ways to fix this:

  • Ignore the execute key if remote plan is detected
  • Cry out and fail with an error if both keys are present

@lukaszachy, @thrix, @happz, @adiosnb, thoughts?

@adiosnb
Copy link
Collaborator

adiosnb commented Oct 27, 2022

I would prefer the cry-and-fail option. What do you think about enforcing correct plans and tests according to schemas?

@psss
Copy link
Collaborator Author

psss commented Oct 27, 2022

What do you think about enforcing correct plans and tests according to schemas?

I think we are getting close to that. Not sure whether we should rather wait for tmt-2.0 as it will definitely cause some failures for existing CI configs.

@jscotka
Copy link
Collaborator

jscotka commented Oct 27, 2022

This is also connected with defining how to stop inheritance for node, as discussed. so that if there will be specific way how to say this node will not inherit from parent anything.
So that then could be said that better way is to use something like and describe, that fmf inheritance + remote plans could have side effects:

plan:
"force_clean_node": true
    import:
        url: https://github.com/teemtee/tests
        name: /plans/polarion

because stop inheritance for remote plans may be contraproductive, e.g. imagine you want to import more plans from one repo like

url: https://github.com/teemtee/tests
plan1:
    import:
        name: /plans/x1
/teemtee/tests
plan2:
    import:
        name: /plans/x2
...

@psss
Copy link
Collaborator Author

psss commented Oct 27, 2022

There already is support for disabling inheritance in fmf, see:

And this should nicely work with the two-plan example above as well.

@jscotka
Copy link
Collaborator

jscotka commented Oct 27, 2022

There already is support for disabling inheritance in fmf, see:

* https://fmf.readthedocs.io/en/latest/features.html#inheritance

And this should nicely work with the two-plan example above as well.

Ah, this sounds good, I've didn't know it is already implemented.
so maybe we should start with documentation to enforce users they will be warned to use this inherit: false as the best solution.

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

No branches or pull requests

3 participants