update lib.sh to change variables to functions #1247

szaimen · 2020-05-17T15:00:59Z

No description provided.

szaimen · 2020-05-17T21:50:18Z

I've already tested if the concept works, which is the case. Hence marking as ready for review.

enoch85 · 2020-05-18T08:34:56Z

The reason why we use the current way is that we do not want to run the variables every time the lib.sh is fetched.

I haven't tested this, so I can't say, but are you 100% sure that the variables aren't set even if it's in a function?

lib.sh

szaimen · 2020-05-18T09:42:49Z

The reason why we use the current way is that we do not want to run the variables every time the lib.sh is fetched.

I haven't tested this, so I can't say, but are you 100% sure that the variables aren't set even if it's in a function?

yes, that is the case. I have tested it by running bash -x on a script (and see which variables get called during the pull of the new library)

This was inside the script:

#!/bin/bash
# shellcheck source=lib.sh
. <(curl -sL https://raw.githubusercontent.com/nextcloud/vm/change_variables_to_functions/lib.sh)

if you run that with bash -x , you can see that no one of these variables inside the new functions get printed out during the initial pull.

Additionally if you e.g.
echo "$CURRENTVERSION", nothing gets printed out because Currentversion wasn't set to a value.

@szaimen

we should add a script that fetches the latest `server_configuration` script @szaimen That way we don't need to keep it in the scripts folder, and it would always be the latest version of the script

enoch85 · 2020-05-22T08:56:31Z

I tested this yesterday, just to see if it works to use functions instead, and it does.

But, some variables are nested into different scripts, and I'm very careful here, since this is the way we done it for the past years. Double, tripple, and quadrouple check all the variables that are changed here. grep -r $VARIABLE for all the variables run when this is set.

I know that download_verify_nextcloud_stable use $NCVERSION, which is used in nc_update, but is that really set? I'm also not 100% sure about checking $CURRENTVERSION

And third, now we no longer have a way to check if the needed functions are actually loaded, which ww check in --------------------------------^.

Thanks!

szaimen · 2020-05-22T09:39:52Z

static/main_menu.sh

        fi
    ;;
    "Server Configuration")
        if network_ok
        then
-            run_static_script server_configuration
+            run_script STATIC server_configuration


Can we please revert all changes in this commit?
As I said it will complicate things to fix #11 by a lot!

I've reverted that.

szaimen · 2020-05-22T09:46:00Z

But, some variables are nested into different scripts, and I'm very careful here, since this is the way we done it for the past years. Double, tripple, and quadrouple check all the variables that are changed here. grep -r $VARIABLE for all the variables run when this is set.

I don't get what you mean. Have you seen, that we have actually a backup for the legacy way of calling them to remain backwards compatibility and in the case that we have forgotten one?
looke e.g. here: https://github.com/nextcloud/vm/pull/1247/files#diff-68f7bc5e839823a7568557cf9e87f4e2R41

I know that download_verify_nextcloud_stable use $NCVERSION, which is used in nc_update, but is that really set? I'm also not 100% sure about checking $CURRENTVERSION

you can just test it. In my testing it works without any flaw.

And third, now we no longer have a way to check if the needed functions are actually loaded, which ww check in --------------------------------^.

Edit: where have we anywhere tested if functions get loaded?
You could just test, if the variables you want to retrieve are actually not zero. E.g. [ -n "$CURRENTVERSION" ] or e.g. [ -n "$NCVERSION" ]

fix #1254

szaimen · 2020-05-22T16:20:00Z

@enoch85 I would like to get this PR in before #1256. Do you think this is doable?

enoch85 · 2020-05-22T21:27:49Z

where have we anywhere tested if functions get loaded?

One example, I'm not sure if there's more: https://github.com/nextcloud/vm/blob/master/lib.sh#L790

Just because what you tested works, it doesn't mean that everything works. Just sayin. Happened to me several times. :)

enoch85 · 2020-05-22T21:32:46Z

I don't get what you mean. Have you seen, that we have actually a backup for the legacy way of calling them to remain backwards compatibility and in the case that we have forgotten one?
looke e.g. here: https://github.com/nextcloud/vm/pull/1247/files#diff-68f7bc5e839823a7568557cf9e87f4e2R41

Ok, nice one. Question is - how long should we keep it? I don't want to clutter the scripts with different approaches. We use one standard, that's it.

szaimen · 2020-05-22T21:43:22Z

I don't get what you mean. Have you seen, that we have actually a backup for the legacy way of calling them to remain backwards compatibility and in the case that we have forgotten one?
looke e.g. here: https://github.com/nextcloud/vm/pull/1247/files#diff-68f7bc5e839823a7568557cf9e87f4e2R41

Ok, nice one. Question is - how long should we keep it? I don't want to clutter the scripts with different approaches. We use one standard, that's it.

The thing is, that we

where have we anywhere tested if functions get loaded?

One example, I'm not sure if there's more: https://github.com/nextcloud/vm/blob/master/lib.sh#L790

Just because what you tested works, it doesn't mean that everything works. Just sayin. Happened to me several times. :)

Okay, but it seems like this is network related, because you just check once in the library and not looping over it. because ncversion will only be set if the website can get reached.

Don't want to say that you don't have a point here but if there was no check before, I won't see why we need to add additional checks, because the functions will work as reliable as the current NC_UPDATE=1 way of getting the needed variables.

szaimen · 2020-05-22T21:45:26Z

I don't get what you mean. Have you seen, that we have actually a backup for the legacy way of calling them to remain backwards compatibility and in the case that we have forgotten one?
looke e.g. here: https://github.com/nextcloud/vm/pull/1247/files#diff-68f7bc5e839823a7568557cf9e87f4e2R41

Ok, nice one. Question is - how long should we keep it? I don't want to clutter the scripts with different approaches. We use one standard, that's it.

What about until the next Ubuntu 22.04 release? I think that is always a good date to get rid of legacy components - also it is nearly 2 years ahead. But that is what you have to decide because many things in the old VMs won't work without that backup.

We could add a comment that it should get removed for Ubuntu 22.04?

enoch85 · 2020-05-22T22:39:41Z

What about until the next Ubuntu 22.04 release?

In general I think that we should keep some backwards compatibility for at least one OS before. So in 20.04 we ditched 16.04. In 22.04 we should ditch 18.04. It's nothing I have written in stone, but I think that's a good approach. It also depends on how much Ubuntu changes ofc. 16.04 to 18.04 was a huge change. 18.04 to 22.04 - not so huge.

But ☝️ is regarding OS versions, I think this is more a matter of keeping backwards compatibility for a shorter period of time, just to make sure that old VMs (like max 6 months) should run without an issue.

I mean the most critical thing here is those who haven't yet deployed their VM. We can expect that they did so within 6 months, right? The update script always uses the latest lib.sh anyway, so that won't be an issue, and people that want to use the latest script for apps and such, it's also fixed with menu.sh already as that will download the latest script in question, already containing the latest lib.sh

What do you think?

enoch85 · 2020-05-22T22:45:04Z

Okay, but it seems like this is network related, because you just check once in the library and not looping over it. because ncversion will only be set if the website can get reached.

Don't want to say that you don't have a point here but if there was no check before, I won't see why we need to add additional checks, because the functions will work as reliable as the current NC_UPDATE=1 way of getting the needed variables.

The whole reason for that change (not using NC_UPDATE for the whole script, and put it in the function instead) is that it takes time to load the variables and it hammers Nextclouds servers more than we need to. IIRC, that's the main reason for it.

But, maybe it's as easy as just check for $CURRENTVERSION as you did, and if if doesn't exist, just run nc_update?

What's making me most uncertain here is that you say that you've tested everything, but then you missed this... In my head "what else did he miss"? But at the same time, that's why we have these discussions. 👍 Even if I'm thorough as ***, I still miss things as well, we are all humans (and all that) :)

szaimen · 2020-05-22T23:12:13Z

Okay, but it seems like this is network related, because you just check once in the library and not looping over it. because ncversion will only be set if the website can get reached.

Don't want to say that you don't have a point here but if there was no check before, I won't see why we need to add additional checks, because the functions will work as reliable as the current NC_UPDATE=1 way of getting the needed variables.

The whole reason for that change (not using NC_UPDATE for the whole script, and put it in the function instead) is that it takes time to load the variables and it hammers Nextclouds servers more than we need to. IIRC, that's the main reason for it.

yes and also we have less variables in every script and it is cleaner to be able to run the functions every time that we need them. Also They wouldn't get called during the script start which would speed up the first start...

What's making me most uncertain here is that you say that you've tested everything, but then you missed this... In my head "what else did he miss"? But at the same time, that's why we have these discussions. 👍 Even if I'm thorough as ***, I still miss things as well, we are all humans (and all that) :)

Actually, I didn't missed it because else I had changed it (if it would have been some of the changed variables). I only haven't remembered now.
But yes, I am a human, too and I can miss things.
That is one of the reason why I have brought in the legacy components like https://github.com/nextcloud/vm/pull/1247/files#diff-68f7bc5e839823a7568557cf9e87f4e2R41

But, maybe it's as easy as just check for $CURRENTVERSION as you did, and if if doesn't exist, just run nc_update?

Yes, that is the whole idea 👍

szaimen · 2020-05-23T09:24:07Z

What about merging this? Or is there still something to discuss?

enoch85 · 2020-05-23T23:02:45Z

Yes, that is the whole idea +1

Great, then please look through the code once more and see if you can find more occurrences.

What about merging this?

No, there are still bugs here, which I mentioned but that isn't fixed.

enoch85 · 2020-05-23T23:08:01Z

PLUS: I don't want a failsafe here. Either it should all work, or we should find were it doesn't and fix t.

szaimen · 2020-05-24T12:24:26Z

Yes, that is the whole idea +1

Great, then please look through the code once more and see if you can find more occurrences.

I've changed download_verify_nextcloud_stable now and I've looked looked for all "[ -z" and "[ -n" and "=1" occurences and there is no old colde that still needs to be fixed, imo (despite of the script in the "old" directory); also the code in download_verify_nextcloud_stable wasn't badly needed to get fixed: it would have worked this way.

This reverts commit e0fff5e.

szaimen and others added 5 commits May 17, 2020 17:00

update lib.sh to change variables to functions

438e22d

fix own travis errors

f552c69

update all scripts to use new functions

7a32346

fix detail

3653c50

fix detail

799606e

szaimen marked this pull request as ready for review May 17, 2020 21:50

change forgotten one

80bc0a0

enoch85 reviewed May 18, 2020

View reviewed changes

lib.sh Show resolved Hide resolved

Merge branch 'master' into change_variables_to_functions

e93e96c

enoch85 added 3 commits May 21, 2020 10:16

Merge branch 'master' into change_variables_to_functions

2578864

change in functions here as well

cabcdc8

we should add a script that fetches the latest `server_configuration` script @szaimen That way we don't need to keep it in the scripts folder, and it would always be the latest version of the script

Merge branch 'master' into change_variables_to_functions

8f1f8ad

szaimen commented May 22, 2020

View reviewed changes

szaimen added 3 commits May 22, 2020 12:54

revert commit cabcdc8

ba7049d

fix #1254

fix forgotten one

5e72b0e

fix forgotten one

d06d534

szaimen linked an issue May 22, 2020 that may be closed by this pull request

change downloading of additional_apps? #1254

Closed

szaimen mentioned this pull request May 22, 2020

update the way scripts are run and downloaded #1256

Merged

This comment has been minimized.

Sign in to view

Merge branch 'master' into change_variables_to_functions

62e20d7

Merge branch 'master' into change_variables_to_functions

6d0d2d4

This comment has been minimized.

Sign in to view

change download_verify_nextcloud_stable

d332fb5

enoch85 merged commit e0fff5e into master May 27, 2020

delete-merged-branch bot deleted the change_variables_to_functions branch May 27, 2020 16:33

enoch85 added a commit that referenced this pull request May 31, 2020

Revert "update lib.sh to change variables to functions (#1247)"

dce4984

This reverts commit e0fff5e.

enoch85 mentioned this pull request May 31, 2020

Revert "update lib.sh to change variables to functions" #1266

Merged

enoch85 added a commit that referenced this pull request May 31, 2020

Revert "update lib.sh to change variables to functions (#1247)" (#1266)

6bb64dd

This reverts commit e0fff5e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update lib.sh to change variables to functions #1247

update lib.sh to change variables to functions #1247

szaimen commented May 17, 2020 •

edited

Loading

szaimen commented May 17, 2020 •

edited

Loading

enoch85 commented May 18, 2020

szaimen commented May 18, 2020 •

edited

Loading

enoch85 commented May 22, 2020

szaimen May 22, 2020

szaimen May 22, 2020

szaimen commented May 22, 2020 •

edited

Loading

szaimen commented May 22, 2020

enoch85 commented May 22, 2020

enoch85 commented May 22, 2020

szaimen commented May 22, 2020

szaimen commented May 22, 2020

enoch85 commented May 22, 2020

enoch85 commented May 22, 2020

szaimen commented May 22, 2020

This comment has been minimized.

szaimen commented May 23, 2020

enoch85 commented May 23, 2020

enoch85 commented May 23, 2020 •

edited

Loading

This comment has been minimized.

szaimen commented May 24, 2020

update lib.sh to change variables to functions #1247

update lib.sh to change variables to functions #1247

Conversation

szaimen commented May 17, 2020 • edited Loading

szaimen commented May 17, 2020 • edited Loading

enoch85 commented May 18, 2020

szaimen commented May 18, 2020 • edited Loading

enoch85 commented May 22, 2020

szaimen May 22, 2020

Choose a reason for hiding this comment

szaimen May 22, 2020

Choose a reason for hiding this comment

szaimen commented May 22, 2020 • edited Loading

szaimen commented May 22, 2020

enoch85 commented May 22, 2020

enoch85 commented May 22, 2020

szaimen commented May 22, 2020

szaimen commented May 22, 2020

enoch85 commented May 22, 2020

enoch85 commented May 22, 2020

szaimen commented May 22, 2020

This comment has been minimized.

szaimen commented May 23, 2020

enoch85 commented May 23, 2020

enoch85 commented May 23, 2020 • edited Loading

This comment has been minimized.

szaimen commented May 24, 2020

szaimen commented May 17, 2020 •

edited

Loading

szaimen commented May 17, 2020 •

edited

Loading

szaimen commented May 18, 2020 •

edited

Loading

szaimen commented May 22, 2020 •

edited

Loading

enoch85 commented May 23, 2020 •

edited

Loading