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

Fix config and start files #336

Merged
merged 7 commits into from Jan 2, 2018

Conversation

Projects
None yet
3 participants
@matsduf
Contributor

matsduf commented Dec 19, 2017

Dedicated backend ini files and start files depending on DB. Updated instructions. This should make it possible to support PostgreSQL for Debian and Ubuntu 16.04.

matsduf added some commits Dec 19, 2017

@matsduf matsduf requested review from mattias-p, sandoche2k and mtoma Dec 19, 2017

@mattias-p

This comment has been minimized.

Show comment
Hide comment
@mattias-p

mattias-p Dec 20, 2017

Contributor

If the CentOS instruction should have separate configs per database engine, I think the other instructions should follow suite. And backend_config.ini without the database suffix should be removed.

The instructions for the various OSes should follow the same outline as that makes for easier maintenance. I put a fair amount of effort into the outline for Debian and FreeBSD. I believe it would be a nice fit for CentOS as well. I probably should have made the update for CentOS at the same time but I didn't. Do you want to update the CentOS instruction outline, or would you rather I do it? I think it should be done.

Contributor

mattias-p commented Dec 20, 2017

If the CentOS instruction should have separate configs per database engine, I think the other instructions should follow suite. And backend_config.ini without the database suffix should be removed.

The instructions for the various OSes should follow the same outline as that makes for easier maintenance. I put a fair amount of effort into the outline for Debian and FreeBSD. I believe it would be a nice fit for CentOS as well. I probably should have made the update for CentOS at the same time but I didn't. Do you want to update the CentOS instruction outline, or would you rather I do it? I think it should be done.

@sandoche2k

This comment has been minimized.

Show comment
Hide comment
@sandoche2k

sandoche2k Dec 20, 2017

Contributor
Contributor

sandoche2k commented Dec 20, 2017

@mattias-p

This comment has been minimized.

Show comment
Hide comment
@mattias-p

mattias-p Dec 20, 2017

Contributor

Here's a CentOS installation instruction that's been updated to use the same outline as Debian and FreeBSD: https://github.com/mattias-p/zonemaster-backend/blob/centos-installation/docs/Installation.md

This far it's a pure cut-and-paste job. I have not yet tested the instruction.

My view is that the different-configs-for-different-dbs change in this PR is moot with the update outline. The updated outline version still does not have instructions for PostgreSQL.

Contributor

mattias-p commented Dec 20, 2017

Here's a CentOS installation instruction that's been updated to use the same outline as Debian and FreeBSD: https://github.com/mattias-p/zonemaster-backend/blob/centos-installation/docs/Installation.md

This far it's a pure cut-and-paste job. I have not yet tested the instruction.

My view is that the different-configs-for-different-dbs change in this PR is moot with the update outline. The updated outline version still does not have instructions for PostgreSQL.

@sandoche2k

This comment has been minimized.

Show comment
Hide comment
@sandoche2k

sandoche2k Dec 20, 2017

Contributor
Contributor

sandoche2k commented Dec 20, 2017

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Dec 20, 2017

Contributor

@mattias-p, I have updated the PR to match your request of having the same format of CentOS as Debian.

Contributor

matsduf commented Dec 20, 2017

@mattias-p, I have updated the PR to match your request of having the same format of CentOS as Debian.

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Dec 20, 2017

Contributor

The reason that I keep backend_config.ini is that it shows commented options etc.

Contributor

matsduf commented Dec 20, 2017

The reason that I keep backend_config.ini is that it shows commented options etc.

Show outdated Hide outdated docs/Installation.md
@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Dec 20, 2017

Contributor

@sandoche2k! Can you review this PR? And approve if you approve. Should @mtoma also look at it? It contains no changes in code.

Contributor

matsduf commented Dec 20, 2017

@sandoche2k! Can you review this PR? And approve if you approve. Should @mtoma also look at it? It contains no changes in code.

@matsduf matsduf added this to the 2017.4 milestone Dec 20, 2017

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Dec 20, 2017

Contributor

I have now updated the MANIFEST file.

Contributor

matsduf commented Dec 20, 2017

I have now updated the MANIFEST file.

Show outdated Hide outdated docs/Installation.md
cd `perl -MFile::ShareDir -le 'print File::ShareDir::dist_dir("Zonemaster-Backend")'`
sudo install -d /etc/zonemaster
sudo install --mode=755 ./backend_config.ini-mysql /etc/zonemaster/backend_config.ini
sudo install --mode=755 ./zm-centos.sh-mysql /etc/init.d/zm-centos.sh
mkdir "$HOME/logs"

This comment has been minimized.

@sandoche2k

sandoche2k Dec 21, 2017

Contributor

The above command could be in one single place, after the specific database is installed.

@sandoche2k

sandoche2k Dec 21, 2017

Contributor

The above command could be in one single place, after the specific database is installed.

This comment has been minimized.

@matsduf

matsduf Dec 21, 2017

Contributor

Updated.

@matsduf

matsduf Dec 21, 2017

Contributor

Updated.

Update Installation.md
Making all instructions being consistent (over OS and DB).
@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Dec 21, 2017

Contributor

@mattias-p and @sandoche2k! Please review again.

Contributor

matsduf commented Dec 21, 2017

@mattias-p and @sandoche2k! Please review again.

sudo install --mode=755 ./backend_config.ini /etc/zonemaster/
sudo install --mode=755 ./zm-backend.sh /etc/init.d/
sudo install --mode=755 ./backend_config.ini-mysql /etc/zonemaster/backend_config.ini
sudo install --mode=755 ./zm-backend.sh-mysql /etc/init.d/zm-backend.sh

This comment has been minimized.

@mattias-p

mattias-p Dec 21, 2017

Contributor

I suggest that a single service script is used that is common for all supported database engines.

This could be implemented by removing the "mysql" entries from "Required-Start" and "Required-Stop", and adding entries for all supported dbms daemons in "Should-Start" and "Should-Stop". We'll be able to use the same approach on FreeBSD - see the BUGS section in https://www.freebsd.org/cgi/man.cgi?query=rcorder&sektion=8&manpath=freebsd-release-ports.

@mattias-p

mattias-p Dec 21, 2017

Contributor

I suggest that a single service script is used that is common for all supported database engines.

This could be implemented by removing the "mysql" entries from "Required-Start" and "Required-Stop", and adding entries for all supported dbms daemons in "Should-Start" and "Should-Stop". We'll be able to use the same approach on FreeBSD - see the BUGS section in https://www.freebsd.org/cgi/man.cgi?query=rcorder&sektion=8&manpath=freebsd-release-ports.

This comment has been minimized.

@matsduf

matsduf Dec 22, 2017

Contributor

I think we should consider that for the 2018.1 release.

@matsduf

matsduf Dec 22, 2017

Contributor

I think we should consider that for the 2018.1 release.

Install files to their proper locations:
```sh
cd `perl -MFile::ShareDir -le 'print File::ShareDir::dist_dir("Zonemaster-Backend")'`
sudo install -d /etc/zonemaster
sudo install --mode=755 ./backend_config.ini /etc/zonemaster/
sudo install --mode=755 ./zm-backend.sh /etc/init.d/
sudo install --mode=755 ./backend_config.ini-mysql /etc/zonemaster/backend_config.ini

This comment has been minimized.

@mattias-p

mattias-p Dec 21, 2017

Contributor

I suggest that a single config file is used that is common for all supported database engines.

This could be implemented with a sed one-liner in the database configuration section to update the config file. Just the way it works without this PR.

I can see one downside with the one-liner approach:

  1. It wouldn't work for SQLite with the current configuration format. That's not a problem right now anyway since we don't provide instructions for SQLite on any OS. #343 eliminates this problem altogether.

I can see two upsides with the one-liner approach:

  1. We'd have fewer files to maintain.
  2. Advanced users would have handy one-liners to switch the configuration between database engines.

If the one-liner approach is taken as well as the combined service script approach (as suggested in my next comment), this sub-section could be deduplicated and moved from section 2.2.* to back up the 2.1 section. I can see no downsides to that but two upsides:

  1. Fewer repetitions for us to maintain in the installation document.
  2. Fewer irrelevant steps for users to skip over in a document that's fairly complex and easy to get lost in.

Of course if there are heavier arguments in favor of separate config files, we should go with that approach.

@mattias-p

mattias-p Dec 21, 2017

Contributor

I suggest that a single config file is used that is common for all supported database engines.

This could be implemented with a sed one-liner in the database configuration section to update the config file. Just the way it works without this PR.

I can see one downside with the one-liner approach:

  1. It wouldn't work for SQLite with the current configuration format. That's not a problem right now anyway since we don't provide instructions for SQLite on any OS. #343 eliminates this problem altogether.

I can see two upsides with the one-liner approach:

  1. We'd have fewer files to maintain.
  2. Advanced users would have handy one-liners to switch the configuration between database engines.

If the one-liner approach is taken as well as the combined service script approach (as suggested in my next comment), this sub-section could be deduplicated and moved from section 2.2.* to back up the 2.1 section. I can see no downsides to that but two upsides:

  1. Fewer repetitions for us to maintain in the installation document.
  2. Fewer irrelevant steps for users to skip over in a document that's fairly complex and easy to get lost in.

Of course if there are heavier arguments in favor of separate config files, we should go with that approach.

This comment has been minimized.

@matsduf

matsduf Dec 22, 2017

Contributor

With your solution both copying the file and editing it with sed (or something else) are required in some instances. I think the solution proposed in my PR gives a cleaner, simpler and more consistent instructions. What is your opinion, @sandoche2k?

When #343 is implemented we can change it again.

@matsduf

matsduf Dec 22, 2017

Contributor

With your solution both copying the file and editing it with sed (or something else) are required in some instances. I think the solution proposed in my PR gives a cleaner, simpler and more consistent instructions. What is your opinion, @sandoche2k?

When #343 is implemented we can change it again.

sudo /etc/init.d/zm-centos.sh status
```
### 1.4 Post-installation sanity check (CentOS)

This comment has been minimized.

@mattias-p

mattias-p Dec 21, 2017

Contributor

The post-installation sanity check sections are repeated and could be deduplicated and moved to a common section like this: https://github.com/dotse/zonemaster-engine/blob/master/docs/Installation.md#post-installation-sanity-check.

@mattias-p

mattias-p Dec 21, 2017

Contributor

The post-installation sanity check sections are repeated and could be deduplicated and moved to a common section like this: https://github.com/dotse/zonemaster-engine/blob/master/docs/Installation.md#post-installation-sanity-check.

This comment has been minimized.

@matsduf

matsduf Dec 22, 2017

Contributor

@mattias-p, I both agree and disagree. It could be moved, but I think the flow of installation would be worse. It is a important part of installation. I do not think that the duplication is such a problem. I suggest that we keep it as is. What do you say, @sandoche2k?

@matsduf

matsduf Dec 22, 2017

Contributor

@mattias-p, I both agree and disagree. It could be moved, but I think the flow of installation would be worse. It is a important part of installation. I do not think that the duplication is such a problem. I suggest that we keep it as is. What do you say, @sandoche2k?

@mattias-p

This comment has been minimized.

Show comment
Hide comment
@mattias-p

mattias-p Jan 2, 2018

Contributor

In response to several comments, the goal I'm trying to achieve with my suggestions is to ease the maintainability of the documentation. Deduplication is an effective means to that end.

Contributor

mattias-p commented Jan 2, 2018

In response to several comments, the goal I'm trying to achieve with my suggestions is to ease the maintainability of the documentation. Deduplication is an effective means to that end.

@mattias-p

This last commit looks good to me.

I'll follow up with a PR that deals with the deduplication I requested in my review comments.

@matsduf matsduf merged commit 090da27 into zonemaster:develop Jan 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matsduf matsduf deleted the matsduf:fix-config-and-start-files branch Jan 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment