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

tec: Remove data/do-install.txt #3555

Merged
merged 3 commits into from
Mar 26, 2021

Conversation

marienfressinaud
Copy link
Member

Changes proposed in this pull request:

  • Remove file data/do-install.txt and its references
  • Remove deleteInstall() method
  • Change install process to start it if data/applied_migrations.txt is missing
  • Update documentation to remove the step asking to delete data/do-install.txt

How to test the feature manually:

  1. git pull new code
  2. check data/do-install.txt is still missing
  3. check FRSS doesn't offer to be reinstalled, even after touch data/do-install.txt
  4. remove data/applied_migrations.txt
  5. check FRSS offers to be reinstalled
  6. run rm data/do-install.txt and touch data/applied_migrations.txt to be in initial state

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard) N/A
  • documentation updated

Additional information can be found in the documentation.

This file was painful during update because we had to remember to delete
it each time. It added a security issue by allowing an attacker to
reinstall FreshRSS during the update process.

The (more powerful) file data/applied_migrations.txt has been introduced
in 8619cf6 to replace do-install.txt. We had to wait for at least one
release in order to make sure existing instances of FreshRSS created the
migration file. It should be ok now.
@Alkarex Alkarex added this to the 1.18.1 milestone Mar 23, 2021
@Alkarex Alkarex merged commit cc6c529 into FreshRSS:edge Mar 26, 2021
@pthall
Copy link

pthall commented May 16, 2021

Hi, I am hosting FreshRSS with Heroku, which wipes local filesystem changes on a regular basis.

After upgrading FreshRSS from 1.17 to 1.18, the installation screen appeared. And whenever Heroku did its periodic filesystem wipe, the installation screen returned.

Reading the changes to do-install.txt and applied_migrations.txt, it's unclear which states of these files triggers the installation screen in 1.18.

To avoid the installation screen from re-appearing repeatedly, I removed the applied_migrations.txt line from data/.gitignore and chmod 444 data/applied_migrations.txt. I didn't dig far enough to figure out whether there is some FreshRSS code intentionally deleting data/applied_migrations.txt, or if applied_migrations.txt in data/.gitignore meant that applied_migrations.txt was never created from git push heroku master.

Just want to note this in case others have similar web hosts with only short-lived filesystem persistence. The old do-install.txt was confusing in Heroku, but applied_migrations.txt is potentially more confusing.

@marienfressinaud
Copy link
Member Author

@pthall Heroku should not touch data/* folder which stores important files. In particular, the users' data are here so I'm wondering why your user isn't deleted 🤔

@pthall
Copy link

pthall commented May 17, 2021

Everything persisted under data/ is committed to Git and pushed to Heroku. Heroku is definitely not ideal for FreshRSS hosting.

@marienfressinaud marienfressinaud deleted the tec/remove-data-do-install branch May 17, 2021 17:44
@marienfressinaud
Copy link
Member Author

Indeed, if you have to commit the files under data/ to Git, you should avoid Heroku to deploy FreshRSS :s A better solution would be to mount an external persistent drive on data/, but I don't know if it's possible with Heroku.

Just to clarify how the new applied_migrations.txt file works (do-install.txt no longer exists):

  • installation is triggered if the file is missing (while it was the opposite with do-install.txt)
  • migrations under app/migrations/ are executed and added to applied_migrations.txt if there are not in it yet. So you should save the state of this file to avoid executing migrations several times. For now it's empty because there are no migrations

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