-
Notifications
You must be signed in to change notification settings - Fork 20
rehearse: use updateconfig to generate CMs #127
rehearse: use updateconfig to generate CMs #127
Conversation
stevekuznetsov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbguimaraes you want to make sure this is ready to go for the profiles and then do templates in another PR?
|
I am considering it. The template changes are almost done, they would make the PR significantly larger and are mostly orthogonal. It would be nice to get #122 in before this so I can add proper integration tests, I'll take a look and see what's left in there. |
|
LGTM, nice! Would prefer to merge this and have another PR for templates... |
|
Rebased and updated. If you've reviewed this before, I tried to keep the diff to a minimum: check only the last commit. I'll squash when approved. This can be merged as is, but you can also check the template PR: Main differences from the previous version:
|
|
Incorporated @petr-muller's suggestion and added an extra test fix that got lost during the refactorings. |
|
Rebase and squashed. Some small changes were necessary because of an upstream change to the interface of the |
stevekuznetsov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bbguimaraes, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I am still working on templates, as that will require more fundamental changes, but the basic mechanism is already reviewable.