Skip to content
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

[enh] Decouple the regen-conf mechanism from services #653

Merged
merged 15 commits into from Apr 18, 2019

Conversation

Projects
None yet
2 participants
@alexAubin
Copy link
Member

commented Feb 18, 2019

The problem

This is a rewriting of #480 because the original PR got stale after too many changes affecting the codebase as a whole (operation logs, error handling, ...)

N.B. : I intend to merge this for the 3.5 release ... Otherwise it's gonna be the same story all over again :/

Original problem :

Regen-conf is currently heavily coupled to the notion of services, and this is problematic / annoying for many reasons :

  • we have "pseudo-services" like "ssl", "nsswitch" and "yunohost" which are not actual services but just here to handle some configuration files
  • it's annoying to create a whole service, then add all the hack to make it "not a real service" when you in fact just want to manage a single file. This is why I ended up putting the management of etckeeper.conf inside the "yunohost" regen-conf hook
  • we have other examples of files we would like to manage independently of the notion of service, for instance the dns resolvers (c.f. #352 ) and cron jobs (dyndns, cert-renewal)

Solution

Decouple the regen-conf mechanism from the concept of services. To do this, the regen-conf related code was moved in a regenconf.py and file hashes are now stored in a regenconf.yml, independently from services.yml.

Of course that's a delicate operation since the regen-conf is one of the core feature of the core :o and a migration is required to properly handle this ... (which is why I ended up working on stretch-unstable to add a 6th migration)

The API also gets changed from yunohost service regen-conf (still supported but deprecated) to yunohost tools regen-conf.

Nevertheless, after this we'll be able to safely add simple conf-regen hooks for new files without all the weird "pseudo-service" thing.

PR Status

Still needs some heavy testing to make sure this doesn't break anything, but should be relatively stable so can be reviewed

How to test

Pull this branch ... the first time you run a regen-conf, it should automatically migrate. Though we also need to test postinstall and a "real-life upgrade" (which triggers the postinst script and so on)

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@alexAubin alexAubin changed the title Decouple regenconf from services [enh] Decouple the regen-conf mechanism from services Feb 18, 2019

@alexAubin alexAubin added this to the 3.5.x milestone Feb 18, 2019

@alexAubin alexAubin referenced this pull request Feb 18, 2019

Closed

Decouple the regen-conf mechanism from services #480

0 of 4 tasks complete
@@ -43,14 +43,16 @@
logger = log.getActionLogger('yunohost.regenconf')


# FIXME : those ain't just services anymore ... what are we supposed to do with this ...

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 1, 2019

Member

Can't we migrate those?

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 1, 2019

Author Member

You mean the existing logs ? Meh I guess it's not so much of a big deal to not migrate them, usually you only care about the recent logs

I was more thinking about if we can change this 'service' key in the is_unit_operation and I guess we can indeed replace it to something like configuration ?

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 1, 2019

Member

or "regen-conf"?

with open('/etc/yunohost/services.yml', 'r') as f:
services = yaml.load(f)
with open(REGEN_CONF_FILE, 'r') as f:
return yaml.load(f)
except:

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 1, 2019

Member

"except:" T_T

(I know it's not related to this PR)

for name in names:
if name not in services:
raise YunohostError('service_unknown', service=service)
if name not in services.keys():

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 1, 2019

Member

Misc: the .keys() is not needed here normally (if it's a dict "a in some_dict" == "a in some_dict.keys()"

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

I'm not 100% sure but shouldn't we ship a default regenconf.yaml file like we do for service for new installations? It won't be created since we don't run migrations on installation.

Or those this magic code is going to create it? 0ebbb83#diff-3cee47e1d870699f286cf66e3a4e04d9R67

@Psycojoker
Copy link
Member

left a comment

It's quite hard to review this PR as a whole since it's very big but I'm totally for this modification so we can move forward with it with more testing and so on.

(except if we did need a default "regenconf.yaml" ^^')

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

I'm not 100% sure but shouldn't we ship a default regenconf.yaml file like we do for service for new installations? It won't be created since we don't run migrations on installation.

Ah yes indeed, we should probably do something explicitly during the postinstall. I forgot to look into this but I doubt that just 0ebbb83 will take care of it

@alexAubin alexAubin modified the milestones: 3.5.x, 3.6.x Mar 9, 2019

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

Alrighty, ran a few tests and fixed a few things. Both the migration and fresh postinstall seem to work fine and behave as expected so it seems ready for testing.

As discussed in meetings a few weeks ago, I'm merging this as we're at the beginning of 3.6 so we'll have time to fully validate this in testing until the stable release

@alexAubin alexAubin merged commit d36c091 into stretch-unstable Apr 18, 2019

2 checks passed

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

@alexAubin alexAubin deleted the decouple-regenconf-from-services branch Apr 18, 2019

@alexAubin alexAubin referenced this pull request May 10, 2019

Merged

[enh] Simplify the whole LDAP interface thing #721

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.