Print changes to be deployed or reverted. #702
Conversation
| @@ -269,6 +274,15 @@ sub deploy { | |||
| $self->$meth( $plan, $to_index ); | |||
| } | |||
|
|
|||
| # Do a thing similar to Sqitch::Plan::Change::format_name_with_tags, | |||
There was a problem hiding this comment.
If you can't use that actual method, I suggest adding a new one there, not here.
There was a problem hiding this comment.
We do not have a Sqitch::Plan::Change here, so I don't think this code can go in that module?
There was a problem hiding this comment.
I do wonder if we should avoid the overhead when verbosity is less than two. Though it probably isn't that expensive, relative to the overhead of the database. Maybe I'm overthinking it.
| @@ -269,6 +274,15 @@ sub deploy { | |||
| $self->$meth( $plan, $to_index ); | |||
| } | |||
|
|
|||
| # Do a thing similar to Sqitch::Plan::Change::format_name_with_tags, | |||
| @@ -269,6 +274,15 @@ sub deploy { | |||
| $self->$meth( $plan, $to_index ); | |||
| } | |||
|
|
|||
| # Do a thing similar to Sqitch::Plan::Change::format_name_with_tags, | |||
There was a problem hiding this comment.
I do wonder if we should avoid the overhead when verbosity is less than two. Though it probably isn't that expensive, relative to the overhead of the database. Maybe I'm overthinking it.
|
Regarding performance, my assumption would be that the overhead in printing would be orders of magnitude smaller than the actual time to deploy the change, even for trivial changes. |
|
Also I'm not sure what the next step is on this PR, do you want to run workflows? Is it ready to merge? |
|
Yeah, I'm overthinking it on performance :-) I've approved the workflows; is there a test that demonstrates that those lines do not output when verbosity is < 2? If so I think it's good to go. |
If you are referring to the If you are referring to the |
|
Regarding these CI failures, I'm not sure what to make of them. They don't seem related to this change? Are the tests a bit flaky? |
|
Yeah I probably need to re-enable Snowflake and figure out what Exasol's problem is. |
|
What is the next step on this PR? Is it ready to merge? |
|
I'll take a look at getting it merged soon, hopefully this weekend, but maybe not for a week or two. |
Closes #701