opengisch / pum

Postgres Upgrades Manager
GNU General Public License v2.0
30 stars 7 forks source link

Add verbose param to upgrade and test-and-upgrade #56

Closed elemoine closed 5 years ago

elemoine commented 5 years ago

This PR adds a --verbose (-vv) argument to the upgrade and test-and-ugrade subcommands. This is to display extra information to the user.

Currently, in verbose mode, the upgrader will display the delta files that are found in the deltas dirs, whether each delta file was already applied, and if the delta version is greater than or equal to the database version. This is useful information to the user already.

3nids commented 5 years ago

I wonder if we should not harmonize the arguments and use -v for verbose and -e for environment variables? This means updating mainly QGEP and QWAT I guess (use of -v for variables). Otherwise, I'm fine with it

elemoine commented 5 years ago

I also wondered about argument consistency. But I did not want to break people's scripts. I actually wouldn't break people's scripts for one-letter flags that are very hard to keep consistent anyway. I am surprised that you don't want to break the internal API and you are ok with breaking the CLI :-) I think you'll break more people by changing the CLI.

3nids commented 5 years ago

Yep that's quite an argument ;) I just tend to prefer to keep default values, I would also prefer to have a default value for verbose. But that can be added later if required. I'm fine with this.

3nids commented 5 years ago

@marioba ok for you?

marioba commented 5 years ago

Personally at the moment I would only make changes that involve no break for current users. That is, last argument with default value in the python function and new option via command line.

I think that, if we decide to change API or options, it makes sense to make all the changes at once, and put it in an ad hoc release (maybe at the same time as some big changes to pum), and clearly specify that it breaks compatibility with older versions.

elemoine commented 5 years ago

And you want to keep the default values for the variables and max_version arguments I can move the verbose argument to the end and give it a default value as well.

marioba commented 5 years ago

Yes, so we don't break compatibility.

elemoine commented 5 years ago

Personally at the moment I would only make changes that involve no break for current users. That is, last argument with default value in the python function and new option via command line.

I think that, if we decide to change API or options, it makes sense to make all the changes at once, and put it in an ad hoc release (maybe at the same time as some big changes to pum), and clearly specify that it breaks compatibility with older versions.

OK, no problem.

Again I am surprised that you treat Pum as a library and don't want to change the signatures of functions within the code. And that makes me wonder what is part of the API and what is not. Are you saying that all the run_ functions are part of the public API? Anything else?

elemoine commented 5 years ago

That is, last argument with default value in the python function

Done.

marioba commented 5 years ago

The initial idea was that the Checker, Dumper and Upgrader classes (with all "public" methods) could be used via python instead of CLI. But I don't know if anyone has ever used pum in this way and if it actually makes sense to worry about that.

elemoine commented 5 years ago

The initial idea was that the Checker, Dumper and Upgrader classes (with all "public" methods) could be used via python instead of CLI.

Ok, but note that I never meant to modify those. I only meant to modify the signatures of the run_ functions in scripts/pum.

marioba commented 5 years ago

Yeah, you're right, I'm sorry, I didn't look carefully. So no problems for the signatures.

@3nids, do you agree that scripts/pum is just the entry point for the CLI and should not be used if you want to use pum via Python?

elemoine commented 5 years ago

Yeah, you're right, I'm sorry, I didn't look carefully. So no problems for the signatures.

In that case I think we should remove the default values for the run_ functions arguments as they don't make too much sense – they are not used.

3nids commented 5 years ago

Yes sorry same misreading in my side. Please go ahead

elemoine commented 5 years ago

As discussed I removed all the unused default values.

3nids commented 5 years ago

Thanks a lot. New release coming in next minutes

elemoine commented 5 years ago

Wait I have another request… :-) In another ticket.