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

[Documentation] Rework Upgrade Guide and add Section "Upgrade from LanSuite v4.2 to v5.0" #794

Merged
merged 18 commits into from
Feb 11, 2024

Conversation

andygrunwald
Copy link
Collaborator

What is this PR doing?

I recently upgraded an older LanSuite instance.

For this, I had first-hand experience on what steps are necessary to upgrade.

This Pull Request reworks the Upgrade Guide a bit:

  • Add specific instructions on v4.2 to v5.0
  • Made it a bit more concise (e.g. i think it is not our responsibility to inform the admin if he/she should inform their users)
  • Removed the git master option - IMO, this should only be done by users who know what they are doing

Which issue(s) this PR fixes:

None

Checklist

  • CHANGELOG.md entry - Not needed
  • Documentation update

@M4LuZ
Copy link
Collaborator

M4LuZ commented Nov 16, 2023

@mrflobow contacted me because he apparently had issues in updating his dev environment while following the guide.
(which indicates that some more work is apparently needed)
I've asked him to post the issues and questions here, so it can be added.

Copy link

sonarcloud bot commented Nov 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@M4LuZ
Copy link
Collaborator

M4LuZ commented Nov 18, 2023

While we wait:

Made it a bit more concise (e.g. i think it is not our responsibility to inform the admin if he/she should inform their users)

The guide lists all steps that should be done to avoid issues before/during/after the upgrade.
And lack of communication / planning is one very often seen.

So I would leave it in.
I'm confident that readers can figure out for themselves if they need to do that or not

@mrflobow
Copy link
Contributor

Hello Andy and Maluz

I would like to get the DevSetup running I tried the steps on the offical instructions but I'm not able to get it running.

My comments

DEV Setup?

git clone https://github.com/lansuite/lansuite.git
cd lansuite
# Erstelle eine Konfigurations Datei in ./inc/base/config.php , eine Beispiel Konfiguration kannst du unten entnehmen
touch ./inc/base/config.php
# > Not sure if applicable for dev setup
chmod 0777 ./inc/base/config.php
# > Would case 10000 commit files.
chmod -R 0777 ./ext_inc/
# Added step to get mysql setting
cp docker/mysql.env.dist docker/mysql.env
#> Image must be build otherwise its taking an image 3years old with php7.x
docker build . -t lansuite/lansuite --platform linux/amd64
#> Add -d for background
docker-compose up -d
docker-compose run php composer install

# Add
# Page is reachable add http://localhost:8080 

Sample should match with mysql.env for lazy devs :D

[lansuite]
version=Nightly
default_design=simple
chmod_dir=777
chmod_file=666
debugmode=0

[database]
server=mysql
user=lansuite
passwd=dev1234
database=lansuite
prefix=ls_
charset=utf8
Bildschirmfoto 2023-11-18 um 12 32 57
Bildschirmfoto 2023-11-18 um 12 33 28
Bildschirmfoto 2023-11-18 um 12 34 25

Not sure what todo next.

Copy link

sonarcloud bot commented Dec 31, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

website/docs/guides/guides-upgrade.md Outdated Show resolved Hide resolved
website/docs/guides/guides-upgrade.md Show resolved Hide resolved
* The base configuration file from `inc/base/config.php`
* Anything under `ext_inc`
* Custom designs from `designs/`

Copy them to the same place on the new installation, overwriting everything there.

### Check file access rights

Depending on the user you did the installation with it may be required to reset the file and folder access rights.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the rationale of removing this?
Unless file operations are done in scope of the webserver user, ownership may be wrong, so may be access rights

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The installation script checks the file permission and warns the user. Thats why I don't think it should be a manual step. On top of this it doesn't say

  • which file permission to check
  • what are the required file permission

Hence it is providing limited value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The installation script is not executed as part of the instructions.
So that does not help.
If I remember correctly it also does not tell what should be set, just that permissions are not sufficient.

I do agree with you that it would be better to detail what permissions should be set.
But that does not mean that the information that this may need to be done should be removed alltogether.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now looking deeper at the old upgrade instructions:

In der Datei inc/base/config.php den Wert von Configured auf 0 zurücksetzen
Nun die Webseite aufrufen. Es erscheint die Installation. Diese ganz normal durchklicken und die Datenbank Zugangsdaten eingeben jedcoh NICHT DAS HÄCKCHEN setzen, dass die DB löscht. Die Installation sollte nun automatisch updaten.

Huh. If that works it covers most of what is needed.

website/docs/guides/guides-upgrade.md Show resolved Hide resolved
website/docs/guides/guides-upgrade.md Show resolved Hide resolved
website/docs/guides/guides-upgrade.md Show resolved Hide resolved
* Please ensure that export and import of Database images use the same character encoding. Using the same client on the same system should ensure this, but be cautious if dump and import are done on different systems/clients
* MySQL 5.7 changes default behavior for GROUP BY clauses. This may lead to unexpected errors in some cases. See [#117](https://github.com/lansuite/lansuite/issues/117) for details
* The master branch is not usable without pulling in additional resources via composer. You must do this first to obtain a runnable installation!
* You have to run a database table upgrade as an extension of the IP field is required to store logging information
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to provide a conversion logic for existing DB entries in IPv4?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is removed. As far a I know this is not needed anymore. I have updated my system already and never did such a conversion

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a look. It appears that old column values are dropped with the related column change.
So there is nothing to convert once the DB update is done.
Not sure if feature or bug :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing the ipv4 addresses without a proper reason is not gdpr confirm either. In this case, I don't think we need to fix this.

On top of this, people have a database backup. If they really need the IPv4 addresses, there is a possibility to add those.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to add a note that this happens and leave it with that.
log is (supposed to be) cleaned up regularly via cron2, so these should disappear in reasonable time anyways.

Ad GDPR:
The proper reason is legimimate interest and is actually fine as long as it is disclosed transparently (where we fail at the moment).
(Apparently there are also some court cases in DE/AU where dynamic IPs were not considered personal information, but as so often it is not clear-cut)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 929e27a

@andygrunwald
Copy link
Collaborator Author

@mrflobow I had a quick check on your journey. It seem to be relate to the MySQL database.
You start your docker setup in background via

#> Add -d for background
docker-compose up -d

Can we verify that MySQL was booted up successfully?
Or that the user

user=lansuite
passwd=dev1234

has permission to create a database?

@M4LuZ The comment by @mrflobow is related to a development installation, not on how to upgrade an existing LanSuite installation. Hence, I would suggest to open up a new ticket on this.

@M4LuZ
Copy link
Collaborator

M4LuZ commented Feb 10, 2024

@M4LuZ The comment by @mrflobow is related to a development installation, not on how to upgrade an existing LanSuite installation. Hence, I would suggest to open up a new ticket on this.

That's fine by me.

@mrflobow: That issue looks very much like what I noticed with #871 / #872
So maybe the fix is just a git pull away... ;)

@andygrunwald
Copy link
Collaborator Author

@M4LuZ Which items are missing in this PR?

@M4LuZ
Copy link
Collaborator

M4LuZ commented Feb 10, 2024

I've resolved the conversations finished and have only these two remaining:

  • Suggestion: Add a note that IP entries are removed from logs
  • Test if menu-writing and permissions are checked / fixed when setting configured=0 as suggested in legacy upgrade guide. But this can be done within [Upgrade docs] Re-Run menu generation #893 to get this closed

Co-authored-by: M4LuZ <MaLuZ@gmx.net>
Copy link

sonarcloud bot commented Feb 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@andygrunwald andygrunwald merged commit b79f4d4 into master Feb 11, 2024
5 checks passed
@andygrunwald andygrunwald deleted the docs-upgrade branch February 11, 2024 10:18
@mrflobow
Copy link
Contributor

mrflobow commented Feb 11, 2024

@andygrunwald related to install, tried it also in a vm with all service installed . The issue is the use of old db class and new which leads to fail in the wizard . Maybe also due to some config bug Maluz fixed. Yes this is unrelated to upgrade , the inital install fails neither its docker or vm.

I believe it's worth to look in a separate bug into it.
At the moment have not the time to invest further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants