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

[WIP] Adding a way how to edit values in a new template #193

Closed
wants to merge 2 commits into from

Conversation

mruprich
Copy link
Contributor

@mruprich mruprich commented Apr 7, 2020

Adding a way how to solve #140. I am marking this as WIP, since I am not a Python pro and I am sure that there will be comments and ideas during the review.

I implemented validate_args function to check the values that the user wants to add to the template, because I think that such a complicated use case is a little bit too much for click itself.

@psss psss self-assigned this Apr 8, 2020
@psss
Copy link
Collaborator

psss commented Apr 8, 2020

Thanks for providing the patch, @mruprich. During the review I've realized two main issues:

Using the approach with extra args causes that the available options are not displayed in the help message when called tmt plan create --help which is not good from user perspective.

And second, I realized that we have agreed on the options which are relevant only for the discover step. If we would like to add a similar flexibility to other steps that would require a bunch of new options. And we would have to handle possible naming collisions.

So I was thinking about a different approach: What about providing a single option for each step which would allow to override the template default with user content provided as yaml data. It could look like this:

tmt plan create --discover '{how: fmf, url: "https://github/psss/tmt"}'

This would also give a nice flexibility in providing only some parameters as needed.

tmt plan create plans/basic \
    --discover '{name: upstream, how: fmf, url: "https://github/", ref: devel, filter: "tier:0,1"}'
    --discover '{name: fedora, how: fmf, url: "https://fedora/"}'

What do you think?

@mruprich
Copy link
Contributor Author

mruprich commented Apr 8, 2020

That was actually something that I was thinking about as well... The inability to edit other phases. What you are suggesting looks good, I will change the PR. Thanks :)

@psss
Copy link
Collaborator

psss commented Apr 8, 2020

Great, thanks! Note that tmt.utils.yaml_to_dict() might come handy ;-)

@psss
Copy link
Collaborator

psss commented Apr 8, 2020

/packit build
/packit test

@psss
Copy link
Collaborator

psss commented Apr 8, 2020

Our good friend packit revealed a regression. @mruprich, you can use make test to ensure tests are passing before submitting the pull request. Steps to reproduce:

tmt init --template full
tmt init --template full --force

The traceback happened in the code which will be changed, so you don't have to investigate this.

@mruprich
Copy link
Contributor Author

@psss I re-did the PR, now multiple --discover options can be added. In base.py in the edit_template function I am trying to make sure that the values are OK, if you can think of a better way, let me know. Also if you want the edit_template in a different place in the code, I can move it around.

I was thinking if it makes sense to edit this further in the future - maybe add options to edit any of those phases in the template? But AFAICT the discover is the most versatile and interesting at this point. If you think that this would be a good idea, we can discuss it and I'll be happy to get this done but probably in a different issue/PR.

@psss
Copy link
Collaborator

psss commented Apr 27, 2020

/packit build
/packit test

@psss
Copy link
Collaborator

psss commented Apr 27, 2020

Thanks for improving the patch. I'm afraid we cannot check for the valid keys as this will be different in individual plugins. Once we have completely migrated all steps to dynamic plugins we can improve this by introducing a commom way how to verify options. I didn't want to force push to your master so here's a new pull request: #219 Could you please have a look and let me know if works for you?

@psss
Copy link
Collaborator

psss commented Apr 29, 2020

Covered by #219.

@psss psss closed this Apr 29, 2020
@mruprich
Copy link
Contributor Author

I know you already merged this but thx @psss, this is really good.

@psss
Copy link
Collaborator

psss commented Apr 29, 2020

Thank you for outlining the implementation, Michal!

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.

2 participants