Enrivonment variable to force the configuration file. #61

fluca1978 · 2023-07-27T14:49:21Z

The command now will honor the environment variable PGENV_CONFIGURATION_FILE so that, if set, will determine the path to the configuration file to be used during all the operations. This allows to decied which configuration file to use before the pgenv starts working, so for example:

% export PGENV_CONFIGURATION_FILE=~/git/pgenv/config/default.conf
% pgenv build 16beta2

will use the default configuration file instead of a version specific one.

Added also a pgenv config use <version> command that prints the environment variable to export or set in the shell session (but does not set itself), so that it is possible to do

% export $( pgenv config use default )

to obtain the environment changed, and hence ease the "computation" of which file name to use for a specific version.

Close #60

The command now will honor the environment variable `PGENV_CONFIGURATION_FILE` so that, if set, will determine the path to the configuration file to be used during all the operations. This allows to decied which configuration file to use before the `pgenv` starts working, so for example: % export PGENV_CONFIGURATION_FILE=~/git/pgenv/config/default.conf % pgenv build 16beta2 will use the default configuration file instead of a version specific one. Added also a `pgenv config use <version>` command that prints the environment variable to export or set in the shell session (but does not set itself), so that it is possible to do % export $( pgenv config use default ) to obtain the environment changed, and hence ease the "computation" of which file name to use for a specific version. Close theory#60

theory

This works once, but then it overwrites the file, which isn't great. If the user specifies a file to use with PGENV_CONFIGURATION_FILE, pgenv should not overwrite it.

README.md

The subcommand `config path` prints out the configuration file path for the specified version, or the default one if no version is specified. In this way, it is possible to export the environment variable like in the following: $ export PGENV_CONFIGURATION_FILE=$( pgenv config path default ) See theory#61

If the user has defined the PGENV_CONFIGURATION_FILE variable he probably does not want to overwrite a per-PostgreSQL specific configuration file. See <theory#61 (review)>

fluca1978 · 2023-07-31T07:24:29Z

I've modified the code:

config path is the command and now it only prints the configuration file location, not the whole variable
it does not overwrite any specific configuration file if the variable is set.

I'm not sure if not overwriting the configuration file is the right behavior, and I suspect it can confuse the users.

theory

Left a few suggested tweaks and a question, and can confirm that it no longer deletes the file. Yay!

README.md

theory · 2023-08-05T17:30:06Z

README.md

@@ -635,6 +635,8 @@ The `config` command accepts the following subcommands:
           (Using `$EDITOR`, e.g: `export EDITOR=/usr/bin/emacs`)
 -    `delete` removes the specified configuration
 -    `migrate` is a command used to change the configuration format between versions of `pgenv`
+-    `path` accepts a version number and prints on standard output the path to such version


Suggested change

- `path` accepts a version number and prints on standard output the path to such version

- `path` accepts a version number (or "default") and prints on standard output the path to such version

Well, every config command accepts default, I think it is already clear, but I added as you suggest.

theory · 2023-08-05T17:32:29Z

bin/pgenv

+    # if the user has defined an environment variable PGENV_CONFIGURATION_FILE
+    # I need to use that instead of trying to understand which file to use
+    if [ ! -z "$PGENV_CONFIGURATION_FILE" -a -z "$skip_env" ]; then
+	pgenv_debug "Configuration file overwritten by environment variable PGENV_CONFIGURATION_FILE = $PGENV_CONFIGURATION_FILE"


Does this actually overwrite the file, or do you mean to say here that the setting of PGENV_CONFIGURATION_FILE overrides the calculated configuration file?

bin/pgenv

theory · 2023-08-05T17:33:01Z

bin/pgenv

+# This can be useful to exdport the environment variable
+# to point to a custom path, like this:
+#
+#    % export PGENV_CONFIGURATION_FILE=$( pgenv config path 16neta2 )


Suggested change

# % export PGENV_CONFIGURATION_FILE=$( pgenv config path 16neta2 )

# % export PGENV_CONFIGURATION_FILE="$( pgenv config path 16neta2 )"

bin/pgenv

theory · 2023-08-05T17:46:35Z

The one thing I'm wondering if there is some way to set a variable that means "don't write out a new file". It's implied by setting PGENV_CONFIGURATION_FILE, but perhaps there could also be something like PGENV_AUTO_WRITE_CONFIGURATION that's set to true by default, but if set to false would prevent build and rebuild from writing it out. If you wanted one written, you'd have to use pgenv config write. Or perhaps PGENV_NO_AUTO_WRITE_CONFIGURATION that's false by default would be easier.

What do you think, @fluca1978?

theory · 2023-08-05T17:47:37Z

Actually I kinda thing it ought not to write a new file by default, but it's probably too late to reverse that pattern now.

See <theory#61>

fluca1978 · 2023-08-07T10:46:36Z

The one thing I'm wondering if there is some way to set a variable that means "don't write out a new file". It's implied by setting PGENV_CONFIGURATION_FILE, but perhaps there could also be something like PGENV_AUTO_WRITE_CONFIGURATION that's set to true by default, but if set to false would prevent build and rebuild from writing it out. If you wanted one written, you'd have to use pgenv config write. Or perhaps PGENV_NO_AUTO_WRITE_CONFIGURATION that's false by default would be easier.

What do you think, @fluca1978?

Well, implementing this is quite easy, but I ask myself if we need another environment variable.
Would you like me to implement this before merging? I suspect the default behavior should be to write always, to be retro compatible. I prefer PGENV_AUTO_CONFIG_WRITE set to on by defaul.t

theory · 2023-08-12T23:19:45Z

Well, implementing this is quite easy, but I ask myself if we need another environment variable. Would you like me to implement this before merging? I suspect the default behavior should be to write always, to be retro compatible. I prefer PGENV_AUTO_CONFIG_WRITE set to on by defaul.t

The reason I suggest this is that for those of us who don't want to create a file every time, there is no way to prevent it without always setting an environment variable — because if you put it in the config file, it's too late. Am I right?

I'm fine to look at doing it in a separate PR, totally, and agree about not changing the default now.

fluca1978 · 2023-08-21T06:24:16Z

Sorry, it is not clear to me how to proceed.
So far we have a variable to "drive" (i.e., point to) a specific configuration file when working with pgenv. This work is complete, according to me, so we can merge, right?

The other thing you are asking for is a way to avoid creation of a configuration file at build/rebuild time.
Either we can use an environment variable, like PGENV_GENERATE_CONFIG_FILE=no or we can "alias" the commands like pgenv build-without-config. Last, we can add arguments like pgenv build 16beta2 --no-config. I am wondering if we are creating too many environment variables and we should switch to a more explanatory command line...

theory · 2023-08-22T15:27:44Z

This work is complete, according to me, so we can merge, right?

I think so, yes.

The other thing you are asking for is a way to avoid creation of a configuration file at build/rebuild time.
Either we can use an environment variable, like PGENV_GENERATE_CONFIG_FILE=no or we can "alias" the commands like pgenv build-without-config. Last, we can add arguments like pgenv build 16beta2 --no-config. I am wondering if we are creating too many environment variables and we should switch to a more explanatory command line...

Yes, I think this is worth discussing in a new issue/PR.

Exploit getopt(1) to get command line options availability. The idea is to do a first parse at the very beginning of the script, so that options can be detected and internal and environmental variables can be set accordingly. Then the command line parsing continues as before. Added a new section to the documentation about variables and command line flags to illustrate how both can be combined and used. Close theory#61

thanodnl · 2023-09-05T10:16:00Z

bin/pgenv

+	pgenv_debug "Cannot write configuration file while PGENV-CONFIGURATION_FILE set!"
+	exit 1


This addition is causing the build script to error, signalling a failed build, when building by a default.conf present in the default config directory.

Debug output (removed the build output, but build succeeded)

citus@6c4d7721b906:~$ echo $PGENV_CONFIGURATION_FILE citus@6c4d7721b906:~$ PGENV_DEBUG=1 pgenv build 14.9 [DEBUG] Looking for configuration in /home/citus/.pgenv/config/14.9.conf [DEBUG] Looking for configuration in /home/citus/.pgenv/config/default.conf [DEBUG] Load configuration from [/home/citus/.pgenv/config/default.conf] [DEBUG] Downloading tarball % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 21.1M 100 21.1M 0 0 3751k 0 0:00:05 0:00:05 --:--:-- 3803k [DEBUG] Patching version 14.9 [DEBUG] Patch index file [] [DEBUG] Skipping patch: no index file found! [DEBUG] configure command line [--prefix=/home/citus/.pgenv/pgsql-14.9] + [--enable-debug --enable-depend --enable-cassert --enable-tap-tests CFLAGS=-ggdb -Og -g3 -fno-omit-frame-pointer -DUSE_VALGRIND --with-openssl --with-libxml --with-libxslt --with-uuid=e2fs --with-icu --with-lz4] ... [DEBUG] Configuration file overwritten by environment variable PGENV_CONFIGURATION_FILE = /home/citus/.pgenv/config/default.conf [DEBUG] Cannot write configuration file while PGENV-CONFIGURATION_FILE set! citus@6c4d7721b906:~$ echo $? 1 citus@6c4d7721b906:~$ echo $PGENV_CONFIGURATION_FILE citus@6c4d7721b906:~$

exit with a non-zero exit here is probably not desirable.

Example how this breaks automated workflows: https://github.com/citusdata/citus/actions/runs/6082295223/job/16499766357

It took me a while to understand this was caused by a change in pgenv instead of actual build failures, but running the steps of the build manually pointed me to this change.

Any idea how this should be solved? As you can see from the debug output pgenv assumes the PGENV_CONFIGURATION_FILE is set, although it is not, probably due to the config being resolved.

I believe the previous behaviour was to write the applied config of the build to a version specific file when seeded from a default config file. This is sane, as it makes rebuilds more deterministic.

minor nit; the log line has a small typo in there

Suggested change

pgenv_debug "Cannot write configuration file while PGENV-CONFIGURATION_FILE set!"

exit 1

pgenv_debug "Cannot write configuration file while PGENV_CONFIGURATION_FILE set!"

exit 1

Checking the github actions link in an incognito window seems to indicate that non-contributors can't see the output of the Build and push step, but it fails on exit 1 on pgenv build <version>.

The output pasted above is easier to digest as it has the big wall of text removed and actually contains the debug lines, cause is the same.

See <#61 (comment)>

fluca1978 · 2023-09-05T11:53:28Z

There are different problems here:

reporting an error (i.e., $? not zero) could be a problem for automated test as @thanodnl reported. The commit 7214768 fixes this, but on the other hand now pgenv never reports an error if the configuration file is not written.
not writing a new configuration file after a build (or alike) when the default.conf file is found is somehow the new desired behavior, since the user now have to use explicitly the pgenv config edit to provide her own configuration file. In other words, the PGENV_CONFIGURAION_FILE is automatically set to default.conf if no other file is found.

@thanodnl could you please test the new version and report in a separate issue any thing that I could have missed?

thanodnl · 2023-09-05T15:12:29Z

@fluca1978 Thanks for the quick turnaround (!), the new commit on the mainline works again for automation use-cases. Nevertheless, I have pinned the version to the latest working tag in our environment to prevent future changes to break our workflow :D. Once a new tag is stamped I will update there, but this makes our dev environment builds more consistent.

With regards to the other points

but on the other hand now pgenv never reports an error if the configuration file is not written

I think if it fails to write a file due to an error it would be acceptable to exit with an error code, however, if it doesn't write a file due to a logical decision I think it would be undesirable to exit 1. In the end the desired behaviour has been met.

not writing a new configuration file after a build (or alike) when the default.conf file is found is somehow the new desired behavior, since the user now have to use explicitly the pgenv config edit to provide her own configuration file. In other words, the PGENV_CONFIGURAION_FILE is automatically set to default.conf if no other file is found.

I take no stance on this. I was under the impression that it deliberately created an extra artefact, next to the postgres build, to capture the exact conditions it was built with. If the new desired behaviour is to not do so that is fine with me. It was mostly an observation that a PR that seemingly only added an environment variable to pass in a config file, had the side effect of not writing the version specific file if it was deduced from default.conf. Secondly the message with PGENV_DEBUG=1 set is a bit misleading given it claims PGENV_CONFIGURATION_FILE was set by the user, which it evidently wasn't. Given it's a debug line I don't care much. The exit code however is important now that I try to codify our developer environment :).

I haven't tested other scenario's as we (my team and me) have a shared default.conf as we regularly build new postgres versions, with an intricate set of configure flags, for extension development.

fluca1978 · 2023-09-06T05:51:47Z

Once a new tag is stamped I will update there, but this makes our dev environment builds more consistent.

The change did not gain a release (tag) because there is also #60 under discussion and development, and to some extent these changes are related.
However, it is a good idea to keep a tagged release approach!

I take no stance on this. I was under the impression that it deliberately created an extra artefact, next to the postgres build, to capture the exact conditions it was built with. If the new desired behaviour is to not do so that is fine with me. It was mostly an observation that a PR that seemingly only added an environment variable to pass in a config file, had the side effect of not writing the version specific file if it was deduced from default.conf. Secondly the message with PGENV_DEBUG=1 set is a bit misleading given it claims PGENV_CONFIGURATION_FILE was set by the user, which it evidently wasn't. Given it's a debug line I don't care much. The exit code however is important now that I try to codify our developer environment :).

There is room for improvement here for sure!
The problem is that pgenv uses deeply environment variables, and it is hard to understand if a variable comes from outside the program or from the inside, as in this case.
I think that #60 will help cleaning up the behavior.

theory · 2023-09-11T02:19:31Z

7214768 seems like the right change to me.

fluca1978 mentioned this pull request Jul 27, 2023

Add Support support for additional environment variables #60

Closed

theory requested changes Jul 29, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

fluca1978 added 2 commits July 31, 2023 08:50

Avoid writing the config file when PGENV_CONFIGURATION_FILE is set.

f77853e

If the user has defined the PGENV_CONFIGURATION_FILE variable he probably does not want to overwrite a per-PostgreSQL specific configuration file. See <theory#61 (review)>

theory approved these changes Aug 5, 2023

View reviewed changes

fluca1978 added a commit to fluca1978/pgenv that referenced this pull request Aug 7, 2023

Minor changes.

7d529f7

See <theory#61>

theory closed this Aug 12, 2023

theory reopened this Aug 12, 2023

fluca1978 merged commit 7aa04ca into theory:master Aug 23, 2023

fluca1978 mentioned this pull request Aug 24, 2023

Instrument the creation of the configuration file #62

Closed

fluca1978 mentioned this pull request Aug 31, 2023

[#61] Command line parsing options #63

Closed

thanodnl reviewed Sep 5, 2023

View reviewed changes

fluca1978 added a commit that referenced this pull request Sep 5, 2023

Fix typo in debug message.

ead7b7d

See <#61 (comment)>

fluca1978 added a commit that referenced this pull request Sep 5, 2023

Do not report an abort when not writing the configuration file.

7214768

See <#61 (comment)>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enrivonment variable to force the configuration file. #61

Enrivonment variable to force the configuration file. #61

fluca1978 commented Jul 27, 2023

theory left a comment

fluca1978 commented Jul 31, 2023

theory left a comment

theory Aug 5, 2023

fluca1978 Aug 7, 2023

theory Aug 5, 2023

theory Aug 5, 2023

theory commented Aug 5, 2023

theory commented Aug 5, 2023

fluca1978 commented Aug 7, 2023

theory commented Aug 12, 2023

fluca1978 commented Aug 21, 2023

theory commented Aug 22, 2023

thanodnl Sep 5, 2023

thanodnl Sep 5, 2023

fluca1978 commented Sep 5, 2023

thanodnl commented Sep 5, 2023

fluca1978 commented Sep 6, 2023

theory commented Sep 11, 2023

	- `path` accepts a version number and prints on standard output the path to such version
	- `path` accepts a version number (or "default") and prints on standard output the path to such version

	# % export PGENV_CONFIGURATION_FILE=$( pgenv config path 16neta2 )
	# % export PGENV_CONFIGURATION_FILE="$( pgenv config path 16neta2 )"

		pgenv_debug "Cannot write configuration file while PGENV-CONFIGURATION_FILE set!"
		exit 1

Enrivonment variable to force the configuration file. #61

Enrivonment variable to force the configuration file. #61

Conversation

fluca1978 commented Jul 27, 2023

theory left a comment

Choose a reason for hiding this comment

fluca1978 commented Jul 31, 2023

theory left a comment

Choose a reason for hiding this comment

theory Aug 5, 2023

Choose a reason for hiding this comment

fluca1978 Aug 7, 2023

Choose a reason for hiding this comment

theory Aug 5, 2023

Choose a reason for hiding this comment

theory Aug 5, 2023

Choose a reason for hiding this comment

theory commented Aug 5, 2023

theory commented Aug 5, 2023

fluca1978 commented Aug 7, 2023

theory commented Aug 12, 2023

fluca1978 commented Aug 21, 2023

theory commented Aug 22, 2023

thanodnl Sep 5, 2023

Choose a reason for hiding this comment

thanodnl Sep 5, 2023

Choose a reason for hiding this comment

fluca1978 commented Sep 5, 2023

thanodnl commented Sep 5, 2023

fluca1978 commented Sep 6, 2023

theory commented Sep 11, 2023