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

Add a switch command to pgenv #53

Merged
merged 3 commits into from
May 11, 2022
Merged

Conversation

thanodnl
Copy link
Contributor

@thanodnl thanodnl commented May 4, 2022

The switch command acts mostly like use, however, it does not manage any database for you, it simply changes the version linked and thus in the path of the user.

When developing the citus extension for postgres we often need to manage many different postgres databases running on the same machine (different ports) at once. We have our own tooling to create these databases from the postgres version installed on the path.

By omitting the starting, stopping and initializing of the database we can still use pgenv to manage different versions of postgres on the same machine.

Last years we used a fork of pgenv with some more functionality hacked in. Over time most of that functionality have been better implemented by the pgenv community. The last missing piece for now is a lightweight way of switching which postgres version is on the path without managing a database.

@fluca1978
Copy link
Collaborator

This is interesting, but seems to me it should do the very same thing as use without starting the PostgreSQL version (but stopping the one running if any).
Should we implemented with a subcase of use then?

@thanodnl
Copy link
Contributor Author

thanodnl commented May 4, 2022

I think stopping a database if running makes a lot of sense.

with a subcase of use, do you think about adding a flag (I don't see many flags used within pgenv) or reuse most of the logics from use, simply skipping the starting of the database.

I will push a change with the latter implemented.

EDIT: after implementing the switch as a subcase of use the code became less repetitive, however, use had the behaviour of an error when changing versions while the old version was not running. This error might make sense for use, but less so for switch.

error:

$ pgenv switch 14.2
PostgreSQL 14.1 not running

I don't see an easy way to omit that error message though :(.
@fluca1978 do you have an idea on how to omit the error message while shutting down a non-running version?

@fluca1978
Copy link
Collaborator

Uhm, the error is coming from pgenv_stop_or_restart_instance. However such message seems only informative, to warn the user.
What if we change such message from https://github.com/theory/pgenv/blob/master/bin/pgenv#L927:

echo "PostgreSQL $v not running"

to something like:

echo "WARN: PostgreSQL $v not running" >&2

so that it is clear that the message is not an error for pgenv. After all, you do a switch and the program reminds you the system is not running. What do you think?

Then the switch + use branch should look fine.

@thanodnl
Copy link
Contributor Author

thanodnl commented May 5, 2022

To start with, I am ok with the output as is. Although a bit strange in our workflow, there is nothing problematic with the message.

I would refrain from routing the message to stderr, as it is not really an error. Better to keep it on stdout for now.

That said, it is a bit strange in UX that it would only say something about the old version when switching. Not to say I would want to add more output telling what the new version is. In general I would expect tools to be quiet when things simply work. And only provide information if it needed to course correct to make it work.

eg. instead of logging that postgres wasn't running on the old version I would only tell the user we have detected the old version was running and we are shutting it down. This provides good insights in what the tool was doing, not wat it was not doing. However this would change the normal workflow for people being used to the use command.

Lets merge this as is, and see what others think about its chattyness.

@fluca1978
Copy link
Collaborator

Well, I'm fine with removing messages that told "something did not happen" in favor of what "did happen".

I think this can be merged, and imrpove the messaging later. The only doubt I've is if we need to work on messaging before the new release is done. @theory are you okay with this?

@theory
Copy link
Owner

theory commented May 7, 2022

Maybe emit the message only when in a verbose or debugging mode?

@fluca1978
Copy link
Collaborator

@thanodnl would you mind fixing the documentation as suggested and resolving the pending comments, then we can squash and merge.

@thanodnl
Copy link
Contributor Author

thanodnl commented May 10, 2022

This is interesting, but seems to me it should do the very same thing as use without starting the PostgreSQL version (but stopping the one running if any).
Should we implemented with a subcase of use then?

I don't see any suggestions in this thread, am I simply not seeing them? Or could it be you have unsubmitted comments on the PR?

Once I know what to fix I will do so happily!

@fluca1978
Copy link
Collaborator

Uhm, I see a couple of pending comments regarding documentation. It could be that the approval has removed these comments on your side?
By the way, if you don't see any pending comment please advice, I will merge and fix a few things in the documentation.

@thanodnl
Copy link
Contributor Author

If you see the word pending on the comment like below:

Screenshot 2022-05-10 at 16 37 42

No one, but you, can see the comments. To make them visible you should submit your review from the files tab via the big green button saying Finish Review

README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -570,6 +587,7 @@ $ pgenv help
Usage: pgenv <command> [<args>]

The pgenv commands are:
switch Set the current PostgreSQL version
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would "group" switch with clear and start with use. The former two are related to symlink management, the latters are related to runtime.

Copy link
Contributor Author

@thanodnl thanodnl May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me, I have re-arranged use/start/stop/restart to be together for that reason, and moved, switch/clear below that section.

fluca1978 added a commit to fluca1978/pgenv that referenced this pull request May 11, 2022
see discussion
<theory#53 (comment)>.
If the server is not running and a `stop` command is issued, the
program should not print a verbose message to the terminal indicating
to the user that the server was not runnig. The output should appear
only if the user is in debug mode (PGENV_DEBUG has a value).
@fluca1978
Copy link
Collaborator

Sorry @thanodnl , my fault! Now you should be able to proceed to changes.
Also please rebase, since I've removed the stop message from the output.

The switch command acts mostly like use, however, it does
not manage any database for you, it simply changes the version
linked and thus in the path of the user.
@thanodnl thanodnl force-pushed the feature/switch branch 2 times, most recently from 39b3b32 to e3072a5 Compare May 11, 2022 11:47
@fluca1978 fluca1978 merged commit 6612ffe into theory:master May 11, 2022
@fluca1978
Copy link
Collaborator

Thnaks @thanodnl

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.

3 participants