self-service-password icon indicating copy to clipboard operation
self-service-password copied to clipboard

RPM spec file cleanup

Open davidcoutadeur opened this issue 1 year ago • 10 comments

This issue is to discuss the RPM spec file cleanup first proposed by @xavierba

Original proposed PR:

  • #634
  • #817

davidcoutadeur avatar Mar 07 '24 10:03 davidcoutadeur

Current PR, rebased on master is #847

davidcoutadeur avatar Mar 07 '24 10:03 davidcoutadeur

I have reviewed the PR #847. It seems fine now.

Here is the list of breaking change in the #847 PR, which must be brought to .deb package, and notified in the changelog / in the release notes / documentation:

  • [x] the dependencies are now explicitly listed in the RPM, including the bundled ones.
  • [x] the configuration files are now in /etc/self-service-password directory
  • [x] the previous configuration files present in /usr/share/self-service-password/conf (all .php files) are migrated to /etc/self-service-password/ during the upgrade process. config.inc.php is migrated as a config.inc.php.bak file, all other php file names are preserved.
  • [x] the very old configuration files, present in /usr/share/self-service-password/ are NOT migrated during the upgrade process, and must be upgraded manually. These files have been deprecated since version 0.9, release in 2015 of October. The release note must define how to migrate these configuration files
  • [ ] ~~a default config.inc.local.php is now provided as a configuration file by the package~~
  • [x] smarty is now a required package See smarty4: https://github.com/ltb-project/self-service-password/issues/850
  • [x] some bundled dependencies have been updated. See smarty4: https://github.com/ltb-project/self-service-password/issues/850, bootstrap 5.3: https://github.com/ltb-project/self-service-password/issues/834, and these dependency list: https://github.com/ltb-project/self-service-password/issues/849
  • [x] unit tests are now run by the build process, only for fedora distribution and debian packaging
  • [x] a minimum version of PHP is now required by the package: >=7.3
  • [x] new basic dependency is now required by the package: sed
  • [x] hidden files (.gitignore,...) from bundled dependencies are now removed from the package
  • [x] now, the cache is being cleaned-up during self-service-password upgrade / install

davidcoutadeur avatar Mar 08 '24 18:03 davidcoutadeur

We should discuss a little about providing local config file, I don't understand the need.

coudot avatar Mar 09 '24 10:03 coudot

I am not completely aligned with providing this new local config file.

There are two benefits IMO:

  • self-explain to the users that the good way to configure ssp is to use the local file
  • show a set of minimal values (those in local file) that must be configured before starting

However it may be a little overkill to provide this local file, which is somehow a duplicate from config.inc.php

davidcoutadeur avatar Mar 09 '24 22:03 davidcoutadeur

Yes, it can be provided as doc, but users can choose to modify the main config file which is managed as config by the package, or create a local config to avoid comparing the changes on main config during the upgrade

coudot avatar Mar 10 '24 09:03 coudot

The idea with providing a local config file is to help users toward configuring the app way the project is instructing them to do it, that is, let the default config file unchanged and override variables in the local config file. It also helps to have a tiny number of variable to look at and possibly change, rather than a 430 lines file.

Actually, I would advocate the default config.inc.php file is not really a config file but rather the application's default and as such should not even be writable and moved out of the conf directory. Only the real config file, that is config.inc.local.php, should be in the config directory. However, this is a modification of the application's logic, not a packaging choice to be made.

xavierba avatar Mar 11 '24 10:03 xavierba

In this case the change would be indeed to have a default value php file that will be loaded before the configuration, this indeed a change in the software.

I keep thinking it is interesting to have all configuration parameters in config.inc.php.

And I would prefer not putting config.inc.local.php in packages.

coudot avatar Mar 11 '24 10:03 coudot

Do you mean, put the config.inc.local.php file as a file explicitly added by the package itself ? In which case, I agree, this should be shipped in the git repo itself, if shipped at all.

xavierba avatar Mar 11 '24 10:03 xavierba

I mean this file should not be shipped at all, or only in /usr/share/doc as an example

coudot avatar Mar 11 '24 10:03 coudot

This feature should be ready. Only thing missing is updating the requirement of smarty4. It can be done in https://github.com/ltb-project/self-service-password/issues/850

davidcoutadeur avatar Mar 12 '24 17:03 davidcoutadeur