nextcloud: add default config for maintenance_window_start

[Pilzinsel64]

Fix #2658
Required for #2641

[Pilzinsel64]

However, I'll test the dailies as soon as this is in master.

[scubamuc]

added to wiki documentation
https://github.com/nextcloud-snap/nextcloud-snap/wiki/Configure-config.php#setting-default-maintenance-window

[kyrofa]

It's been a while since we've made a config change, but I'm pretty sure the change you've made here will only effect new installs. Is this something we should be adding for existing installs as well?

[Pilzinsel64]

Is this something we should be adding for existing installs as well?

Yes, of course. Should I put it in autoconfig instead?

[kyrofa]

No, that has the same issue. Honestly, I don't think we've ever been in a situation like this, where ideally we'd patch the current config to have a default value, but the user should also be able to override it. The only thing even close is where we twiddle the redis config. But we don't care if the user set that to something else, because the snap isn't designed for that to be configurable by the user: we just set it directly. In this case... we do care. I need to mull this over a bit, but I'm leaning toward using our migrations capability for this. Thoughts?

[Pilzinsel64]

I'm leaning toward using our migrations capability for this. Thoughts?

Using that migrations capability seems to be perfect for this! I assume they will be called whenever the snap package version is same or higher then the folder version and then it calls every file in alphabetical order?

However, I'm absolutely not fimilar with python.

[kyrofa]

You can see how it works here. It's run via the post-refresh hook.

As you can see, we only have one migration so far. Let's use it to understand how this works.

In snap version 19.0.3snap3, we changed the logging strategy, logging to completely different files. We didn't want to leave old logs lying around, and we wanted the new logs to include the old data, so as part of installing 19.0.3snap3, we wanted to move the old logs to the new location.

First rule: if you have a snap version VERSION where you know that an upgrade from anything older has to have something happen to it in order to be compatible with VERSION, create a new directory within the src/migrations/migrations directory named the same as VERSION.

Second rule: there could be multiple migrations that need to happen in this situation. You can create multiple scripts within the VERSION directory, and they will be run in order (so number them to specify order).

Then, run-snap-migrations will compare the version of the snap before it refreshed, OLD_VERSION, to each of the migration directories. It will run the migrations for any directory in there that represents a snap version NEWER than OLD_VERSION.

Does that make sense? So if we know that this is required for snap version 28.0.0snap1, we create a migration directory named that, and create a new shell script named, I don't know, 1_add-cron-maintenance-window.sh in there.

My hook knowledge is rusty, but I think I just realized a hairy aspect of this: I'm pretty sure post-refresh runs before services are fired back up, so our ability to use occ in here might be pretty limited, depending on how it works (e.g. does it require access to the database?)

[Pilzinsel64]

Thanks for the explination! :) Seems to work mostly like I thought.

I'm pretty sure post-refresh runs before services are fired back up, so our ability to use occ in here might be pretty limited, depending on how it works (e.g. does it require access to the database?)

While maintenance mode we still could edit configs. But if the services aren't even running, we can only modify the file config.php directly.

[Pilzinsel64]

I added such a script. Well, the condition is that we publish a release for the upcomming nc28.0.2. But I am confident with that because the errors in the log are not anymore present in that mass, just after ending maintenance mode. As long as we get the other PR with the javascript mime type done, we can beta-test and probably make a release sooner or later. And if needed, we can always rename the folder, if needed.

I can not test the script today, need to do that tomorrow. Feel free to test the script yourself. But it should work™.

That means, this is ready for review! :)

[Pilzinsel64]

The script works on my prod instance when executing it manually.

[kyrofa]

The script works on my prod instance when executing it manually.

I'd like to see it tested for real, though, in the hooks. It will probably require building this locally, are you setup for that? Also, given the risk of sedding config files, I'd like to see shellspec tests for this.

[Pilzinsel64]

I'd like to see it tested for real

Doesn't that requie a package named like "28.0.0snap1" or higher? Even if built locally? (Sorry, don't have enought knowledge to answer that myself.)

are you setup for that?

I built the snap manually two times in the past. But I always had problems I would need to setup everything again, so I avoid it where I can. 🤐

I'd like to see shellspec tests for this.

😳😥

However, I'm going to take a look into both, test and specs. But sadly I have no free time this week left and can't test it until next week (probably Monday/Tuestday). ;) If someone have too many time left, feel free to take over. Otherwise, sorry for the delay. :/

[pachulo]

Hi @Pilzinsel64 ! Will you have time to check this now that 28.0.4 has been released?

Thanks!

[scubamuc]

@pachulo, don't know if this is relevant, but I did test "set maintenance window" in #2704 as per FAQ is that not default?... not sure what else needs to be done?

added this to testing script also

[Pilzinsel64]

Oh, I am so sorry. I absolutely lost this our of my mind. Also I was werey busy the last weeks.
Tomorrow I'll have some time and I'm going to try to create the shell specs there. :)

[Pilzinsel64]

I'm done. Tests are happy, the script does what it should do and the git history is cleaned. If you have any other suggestions I missed (not unlikely if you can't see the forest for the trees), just tell me.

[Pilzinsel64]

This script works on my private cloud.

Going to test three cases when the build is done and has been published:

• Does the config gets added on a fresh install?
• Does the config gets added on an upgrade from Nextcloud 27 without the config beeing available?
• Does the config doesn't get added on an upgrade from Nextcloud with the config beeing available?

[Pilzinsel64]

Ok, tested now and only a fresh install works. Seems like the script doesn't get executed on an upgrade.
Any ideas? Did i miss something? 🤔

[pachulo]

Any ideas? Did i miss something? 🤔

I think that the src/nextcloud/fixes/existing-install/7_add_maintenance_window.sh lacks the execute bit.

[Pilzinsel64]

Oh come on... Yea, that's it. Will fix it later. 🤦 Thanks!

[Pilzinsel64]

All my tests were successful! 😃 Now I want to request review form one of you once again.

[scubamuc]

fresh install tests completed

snap install nextcloud --channel=latest/beta/pr-2659

• [ x ] default maintenance window was set correctly

---

looking good!

[pachulo]

All my tests were successful! 😃 Now I want to request review form one of you once again.

My opinion is that we can already merge this and then make a call for testing in https://forum.snapcraft.io/, what do you think @kyrofa ?

Besides, to properly test this myself I need for it to be merged and then build into the latest/beta channel, because my production instance is arm64 based.

Thanks a lot for your tenacity @Pilzinsel64 !

[pachulo]

gently ping @kyrofa

[kyrofa]

Yes, this looks excellent, sorry for sending you on a wild goose chase here, but I think we've arrived at the proper solution.

• Does the config gets added on a fresh install?
• Does the config gets added on an upgrade from Nextcloud 27 without the config beeing available?
• Does the config doesn't get added on an upgrade from Nextcloud with the config beeing available?

Just to confirm, one of these last two tested that we don't overwrite an existing maintenance_window_start? Take a quick glance at the logs of the nextcloud-fixer in these situations, too: does getting a key that doesn't exist look like an error in the log? If so, we might receive issues logged for that, and we might consider silencing such a thing. In all though, I'm +1 to this.

[Pilzinsel64]

Just to confirm, one of these last two tested that we don't overwrite an existing maintenance_window_start?

Yes, tested this too. I used config value 5 beeing on latest/stable (so NC27) and then refreshed to latest/beta/pr-2659.

does getting a key that doesn't exist look like an error in the log

No, it doesn't. It just return an exit code 1 wich is catched by the if. And while testing the sh I didn't get an error. But to ensure nothing is in the fixer-log, I quickyl need to install it once again later (need to go out now).

[Pilzinsel64]

does getting a key that doesn't exist look like an error in the log

No, it doesn't. It just return an exit code 1 wich is catched by the if. And while testing the sh I didn't get an error. But to ensure nothing is in the fixer-log, I quickyl need to install it once again later (need to go out now).

Oh, you mean in the Nextcloud log? No, there isn't an error. Also, that even would be increadible as the same function behind is used by Nextcloud itself covered in a function to get the default value if null.

And btw, no error in the fixer log. :)

[kyrofa]

Awesome, let's ship it.