improve config search and add default freebsd config location #835

hannaeko · 2021-07-29T11:17:50Z

Purpose

Improve the config search mechanism

Context

Following the comment #833 (comment)

Changes

iterate through a default config location list to find an existing file
add default freebsd location to the search list (/usr/local/etc/zonemaster/backend_config.ini)
update travis file to correctly install the share directory

How to test this PR

Create the file /etc/zonemaster/backend_config.ini, run the backend, check that this is the config file loaded, then delete it
Create the file /usr/local/etc/zonemaster/backend_config.ini, run the backend, check that this is the config file loaded, then delete it
Create the file /tmp/my_config.ini, set the environment variable ZONEMASTER_BACKEND_CONFIG_FILE to /tmp/my_config.ini, run the backend, check that this is the config file loaded, then delete it

matsduf · 2021-07-29T14:28:18Z

Is it really good to add the number of places that Config.pm "magically" looks for the configuration file? Isn't it better to let e.g. the start script point out by environment variable where to find it?

hannaeko · 2021-07-29T15:24:41Z

The environment variable can still be used, I am just adding one more common place to search to the already existing list Config.pm is using. I think that searching for a configuration in standard places is a common pattern but I might be wrong.

Also those are pretty common places to have the configuration file, I think I prefer to have it specified once here than having to deal with edge cases in multiple location because the ones listed here don't work for an OS that we say we are supporting.

matsduf · 2021-07-29T22:20:40Z

Also those are pretty common places to have the configuration file, I think I prefer to have it specified once here than having to deal with edge cases in multiple location because the ones listed here don't work for an OS that we say we are supporting.

I understand what you mean and I see the benefit of it too. There were some issues before when unit tests used the configuration files under /etc which made them fail when the updates were not backward compatible. I guess that @mattias-p has fixed that issue.

I agree that is simplifies.

matsduf · 2021-07-29T22:23:56Z

.travis.yml

@@ -107,6 +107,10 @@ before_install:
  - git clone --depth=1 --branch=$TRAVIS_BRANCH https://github.com/zonemaster/zonemaster-engine.git
  - ( cd zonemaster-engine && cpanm --verbose --notest . ) && rm -rf zonemaster-engine

+    # Install share files
+  - mkdir -p ./lib/auto/share/dist/
+  - ln -s ../../../../share ./lib/auto/share/dist/Zonemaster-Backend


How does this change relate to this PR?

Otherwise the tests fail as dist_file cannot find the dist directory for Zonemaster-Backend

This PR adds a second path to look for backend_config.ini, else Config.pm still falls back to what dist_file gives. What has changed that requires a change for Travis? Or is it a bug that you fix at the same time?

Before, dist_file was not executed as we are using the environment variable, now it is always executed, that is the difference.

If I may, instead of having to install the share files I'd rather avoid executing dist_file if the ZONEMASTER_BACKEND_CONFIG_FILE variable is set. This would prevent failure such as the Travis one. Would it be possible to execute dist_file only if the environment variable is unset?

Nice, thanks. Then the Travis patch could be removed I guess.

Then the Travis patch could be removed I guess.

I would prefer to keep it so that the share directory is correctly installed.

Before, dist_file was not executed as we are using the environment variable, now it is always executed, that is the difference.

That change is not included in the description. Please add that.

This has been reverted, dist_file is now executed only if the variable is not set, as it was before.

matsduf · 2021-08-02T15:04:22Z

.travis.yml

@@ -107,6 +107,10 @@ before_install:
  - git clone --depth=1 --branch=$TRAVIS_BRANCH https://github.com/zonemaster/zonemaster-engine.git
  - ( cd zonemaster-engine && cpanm --verbose --notest . ) && rm -rf zonemaster-engine

+    # Install share files
+  - mkdir -p ./lib/auto/share/dist/
+  - ln -s ../../../../share ./lib/auto/share/dist/Zonemaster-Backend


This PR adds a second path to look for backend_config.ini, else Config.pm still falls back to what dist_file gives. What has changed that requires a change for Travis? Or is it a bug that you fix at the same time?

matsduf · 2021-08-02T15:07:42Z

lib/Zonemaster/Backend/Config.pm

+        if ( -e $default_path ) {
+            $path = $default_path;
+            last;
+        }


In the current code, $path is always defined. In the new code it will be undefined if the environment variable is undefinded and backend_config.ini does not exist in any of the three paths. That will probably give a different error message. It should probably be defined.

The error will be the same actually. If dist_file('Zonemaster-Backend', "backend_config.ini") fails to find the file, then the backend fails to start with error Failed to find shared file 'backend_config.ini' for dist 'Zonemaster-Backend' in both cases, otherwise $path will be defined as the file exists.

Does the Change Request still holds? It is blocking the merge process so I would like to know if something needs to be addressed.

Please update the description of this PR to include the change for Travis, and I am fine with the changes.

I've updated the description.

matsduf · 2021-12-03T12:11:02Z

Testing v2021.2

LGTM.

As far as I can see it works as it should.

hannaeko added 2 commits July 29, 2021 13:11

improve config search and add default freebsd config location

17c12a0

fix typo

f707413

hannaeko mentioned this pull request Jul 29, 2021

add scripts to backfill undelegated fields in db #833

Merged

3 tasks

update travis

3c2623d

matsduf reviewed Jul 29, 2021

View reviewed changes

matsduf approved these changes Jul 29, 2021

View reviewed changes

matsduf mentioned this pull request Aug 1, 2021

Use a unique set of definitions to initialize the database #834

Merged

matsduf requested changes Aug 2, 2021

View reviewed changes

hannaeko requested a review from matsduf August 4, 2021 11:51

move search path definition

877c8ca

ghost approved these changes Aug 5, 2021

View reviewed changes

matsduf approved these changes Aug 12, 2021

View reviewed changes

hannaeko merged commit e9f4f0e into zonemaster:develop Aug 16, 2021

matsduf added this to the v2021.2 milestone Sep 1, 2021

matsduf added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve config search and add default freebsd config location #835

improve config search and add default freebsd config location #835

hannaeko commented Jul 29, 2021 •

edited

Loading

matsduf commented Jul 29, 2021

hannaeko commented Jul 29, 2021

matsduf commented Jul 29, 2021

matsduf Jul 29, 2021

hannaeko Aug 2, 2021

matsduf Aug 2, 2021

hannaeko Aug 3, 2021

ghost Aug 5, 2021

ghost Aug 5, 2021

hannaeko Aug 5, 2021

ghost Aug 5, 2021

matsduf Aug 9, 2021

hannaeko Aug 10, 2021

matsduf Aug 2, 2021

matsduf Aug 2, 2021

hannaeko Aug 3, 2021

hannaeko Aug 9, 2021

matsduf Aug 9, 2021

hannaeko Aug 10, 2021

matsduf commented Dec 3, 2021

improve config search and add default freebsd config location #835

improve config search and add default freebsd config location #835

Conversation

hannaeko commented Jul 29, 2021 • edited Loading

Purpose

Context

Changes

How to test this PR

matsduf commented Jul 29, 2021

hannaeko commented Jul 29, 2021

matsduf commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Dec 3, 2021

Testing v2021.2

hannaeko commented Jul 29, 2021 •

edited

Loading