openhab / openhabian

openHABian - empowering the smart home, for Raspberry Pi and Debian systems
https://community.openhab.org/t/13379
ISC License
822 stars 251 forks source link

Refactor permissions_corrections() #1561

Closed mstormi closed 2 years ago

mstormi commented 3 years ago

We should intro is_installed_XXX() functions for all 3rd party options we offer as part of openHABian.

Then refactor permissions_corrections() in system.bash using those functions to order / structure the code. It gets more and more convoluted, increasing likelihood to introduce logical mistakes.

Also add permissions_corrections() to all 3rd party BATS tests else packages might be mis-installed hence mis-tested

mstormi commented 2 years ago

@ecdye Renamed it just for you to enjoy. Does that motivate you to work this out ?

ecdye commented 2 years ago

I can certainly work on it, I have just been busy lately I will start on this, I will probably do a PR for each part just to simplify the process.

mstormi commented 2 years ago

Thanks, I'm currently hitting many todos - small ones I fix right away but also some that require more work - so happy for any load you can take off me. Will eventually document some more of these issues.

mstormi commented 2 years ago

On a sidenote I wanted to make you aware that I've just forwarded main to openHAB3 branch. Hope I didn't break anything this way.

That has sort of been overdue anyway and should also fix this interesting issue. I believe installs have been failing on non RPi. Eventually the "wrong architecture java binary" issues I've seen occasionally might also have been caused by this.

openhabian@devpi:~/openhabian $ git checkout main
Zu Branch 'main' gewechselt
Ihr Branch ist auf demselben Stand wie 'origin/main'.
openhabian@devpi:~/openhabian $ git remote -v
myrepo  https://github.com/mstormi/openhabian.git (fetch)
myrepo  https://github.com/mstormi/openhabian.git (push)
origin  https://github.com/openhab/openhabian.git (fetch)
origin  https://github.com/openhab/openhabian.git (push)
openhabian@devpi:~/openhabian $ git push origin openHAB3
Gesamt 0 (Delta 0), Wiederverwendet 0 (Delta 0), Pack wiederverwendet 0
remote:
remote: Heads up! The branch 'openHAB3' that you pushed to was renamed to 'backup.openHAB3'.
remote:
To https://github.com/openhab/openhabian.git
   75187cd..3cdb213  openHAB3 -> openHAB3
openhabian@devpi:~/openhabian $
mstormi commented 2 years ago

Ethan please can you take care to finalize task 3 ?

ecdye commented 2 years ago

I meant to talk to you about that, there is no good way to add the function to all BATS tests, the only really good way to do something similar would be to use the fix_permissions function with the specific files. We can't add the main function because it does not work when openhab is not installed.

mstormi commented 2 years ago

I had thought about modularizing fix_permissions so you can call it with an argument say "grafana" to fix only Grafana related permission, then in BATS tests only call it with test specific argument wdyt?

ecdye commented 2 years ago

That's possible but would significant restructuring of the way the code is written. I think that the better question is, do we need to test that is BATS as it is already being tested in our install tests. I don't think that in most cases it is useful to test changing the permissions of the newly installed program because it typically is only after being installed for a while or rebooting that we have the permissions issues that occasionally crop up.

I would almost prefer to leave it as it is now that its been refractored because that was a big thing more because of the code readability and uniformity.

Thoughts?

mstormi commented 2 years ago

ATM I don't recall why this task is there. Wasn't there a BATS test failing (homegear?) when fix_permissions ain't executed? Remember that's default on every install.

ecdye commented 2 years ago

No, the Homegear test failing is because of bullseye and their devs being slow to fix their package. We never had an issue with permissions being incorrect in BATS IIRC. That's why I am hesitant to invest all the extra effort into the code that already works.

mstormi commented 2 years ago

Alright as you wrote task 3 would require enormous changes and I also don't think it's worth pursuing. So will close this. Guess that should the need for modukarization show up again, we'll recall this issue and can reactivate it.

Have you already embarked on your mission ? How is it going ?