Thread: Minor cleanups in the SSL tests
When writing a new SSL test for another patch it struck me that the SSL tests are doing configuration management without using the test framework API's. The attached patches cleans this up, no testcases are altered as part of this. 0001 makes the test for PG_TEST_EXTRA a top-level if statement not attached to any other conditional. There is no change in functionality, it's mainly for readability (PG_TEST_EXTRA is it's own concept, not tied to library presence). 0002 ports over editing configfiles to using append_conf() instead of opening and writing to them directly. -- Daniel Gustafsson
Attachment
On 16.05.24 09:24, Daniel Gustafsson wrote: > When writing a new SSL test for another patch it struck me that the SSL tests > are doing configuration management without using the test framework API's. The > attached patches cleans this up, no testcases are altered as part of this. > > 0001 makes the test for PG_TEST_EXTRA a top-level if statement not attached to > any other conditional. There is no change in functionality, it's mainly for > readability (PG_TEST_EXTRA is it's own concept, not tied to library presence). Makes sense to me. > 0002 ports over editing configfiles to using append_conf() instead of opening > and writing to them directly. Yes, it's probably preferable to use append_conf() here. You might want to run your patch through pgperltidy. The result doesn't look bad, but a bit different than what you had crafted. append_conf() opens and closes the file for each call. It might be nice if it could accept a list. Or you can just pass the whole block as one string, like it was done for pg_ident.conf before.
> On 16 May 2024, at 11:43, Peter Eisentraut <peter@eisentraut.org> wrote: > You might want to run your patch through pgperltidy. The result doesn't look bad, but a bit different than what you hadcrafted. Ugh, I thought I had but clearly had forgotten. Fixed now. > append_conf() opens and closes the file for each call. It might be nice if it could accept a list. Or you can just passthe whole block as one string, like it was done for pg_ident.conf before. The attached v2 pass the whole block as a here-doc which seemed like the best option to retain readability of the config. -- Daniel Gustafsson
Attachment
On 16.05.24 23:27, Daniel Gustafsson wrote: >> On 16 May 2024, at 11:43, Peter Eisentraut <peter@eisentraut.org> wrote: > >> You might want to run your patch through pgperltidy. The result doesn't look bad, but a bit different than what you hadcrafted. > > Ugh, I thought I had but clearly had forgotten. Fixed now. > >> append_conf() opens and closes the file for each call. It might be nice if it could accept a list. Or you can just passthe whole block as one string, like it was done for pg_ident.conf before. > > The attached v2 pass the whole block as a here-doc which seemed like the best > option to retain readability of the config. Works for me.
> On 17 May 2024, at 07:57, Peter Eisentraut <peter@eisentraut.org> wrote: > > On 16.05.24 23:27, Daniel Gustafsson wrote: >>> On 16 May 2024, at 11:43, Peter Eisentraut <peter@eisentraut.org> wrote: >>> You might want to run your patch through pgperltidy. The result doesn't look bad, but a bit different than what youhad crafted. >> Ugh, I thought I had but clearly had forgotten. Fixed now. >>> append_conf() opens and closes the file for each call. It might be nice if it could accept a list. Or you can justpass the whole block as one string, like it was done for pg_ident.conf before. >> The attached v2 pass the whole block as a here-doc which seemed like the best >> option to retain readability of the config. > > Works for me. Thanks for review. Once the tree opens up for v18 I'll go ahead with this. -- Daniel Gustafsson