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.

Sign up for GitHub

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

Jump to bottom

Replace Ansible with Shell script #307

Merged
merged 1 commit into from
Nov 13, 2022
Merged

Replace Ansible with Shell script #307

merged 1 commit into from
Nov 13, 2022

Conversation

ngosang
Copy link
Contributor

@ngosang ngosang commented Oct 29, 2022 •
edited

  • Remove Ansible and all Python packages
  • Reduce image size by 456 MB (689 MB => 233 MB uncompressed)
  • Fixes some open issues, for example, root password is not required if the database already exists.
  • Show install and startup traces (traces and errors were hidden by Ansible)

Resolves #303 #269 #270 #256

@ngosang
Copy link
Contributor Author

ngosang commented Oct 29, 2022 •
edited

@j0k3r @nicosomb @Kdecherf I did many tests but this change is big. It keeps all the functionality and the environment variables are the same.

@ngosang
Copy link
Contributor Author

ngosang commented Oct 29, 2022 •
edited

Ready to review. If you give me permissions in this repository I can triage the open issues. Half of them are already fixed with the latest changes.

@ngosang ngosang mentioned this pull request Nov 1, 2022
Draft
j0k3r
j0k3r reviewed Nov 2, 2022
View reviewed changes
Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

You need to rebase against master to fix conflict (I guess it's about the composer version).
Maybe @Kdecherf can have a look before merging?

Dockerfile Outdated Show resolved Hide resolved
@j0k3r j0k3r requested a review from Kdecherf November 2, 2022 09:42
@Kdecherf
Copy link
Member

Kdecherf commented Nov 2, 2022

@ngosang @j0k3r fyi I need to take time to review and test these changes, but I won't be able to do it before two weeks

@ngosang
Copy link
Contributor Author

ngosang commented Nov 2, 2022

I can wait, but I hope that at some point it can be merged. It has cost me a lot of effort. If it could be merged before the next release of Wallabag that would be perfect.

@j0k3r
Copy link
Member

j0k3r commented Nov 2, 2022

@ngosang your efforts won't be lost, trust me. That PR will be merged.

The next wallabag version might be 2.6.0 (which will be a technical upgrade of Symfony and also support recent versions of Composer) and it could be great to have all improvements you made on the Docker image at that time 👍🏼

Kdecherf
Kdecherf reviewed Nov 13, 2022
View reviewed changes
Copy link
Member

@Kdecherf Kdecherf left a comment

Choose a reason for hiding this comment

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

Awesome work @ngosang!

I've quickly tested the PR against SQLite, MariaDB and PostgreSQL with 3 scenarios:

  • First boot, uninstalled
  • Reboot with root db credentials still there
  • Reboot without root db credentials

Looks good, I made some comments though

Dockerfile Show resolved Hide resolved
root/entrypoint.sh Outdated Show resolved Hide resolved
root/entrypoint.sh Outdated Show resolved Hide resolved
root/etc/wallabag/parameters.template.yml Show resolved Hide resolved
root/etc/wallabag/parameters.template.yml Show resolved Hide resolved
root/entrypoint.sh Show resolved Hide resolved
@Kdecherf Kdecherf requested a review from j0k3r November 13, 2022 12:35
@Kdecherf
Copy link
Member

Side note: there may be some cleanup to do on the commit log (rewording and/or squashing some commits)

@ngosang
Replace Ansible with Shell script
229cb3d 
* Remove Ansible and all Python packages
* Reduce image size by 456 MB (689 MB => 233 MB uncompressed)
* Fixes some open issues, for example, root password is not required if the database already exists.
* Show install and startup traces (traces and errors were hidden by Ansible)
@ngosang
Copy link
Contributor Author

ngosang commented Nov 13, 2022

@Kdecherf @j0k3r I fixed the typos and squashed all commits into one. I agree with the other suggestions but I think they are out of the scope of this PR. This PR replaces Ansible with bash trying to have the same default values for environment variables. Also this PR is already tested. Remaining changes could be implemented in other PRs.

Kdecherf
Kdecherf approved these changes Nov 13, 2022
View reviewed changes
Copy link
Member

@Kdecherf Kdecherf left a comment

Choose a reason for hiding this comment

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

LGTM

@Kdecherf
Copy link
Member

@ngosang you're right, we'll handle those improvements in separate PR.

Thanks for your work

@Kdecherf Kdecherf merged commit 74bfdfa into wallabag:master Nov 13, 2022
@ngosang ngosang deleted the feature/ansible branch November 29, 2022 23:08
This was referenced Nov 30, 2022
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kdecherf Kdecherf Kdecherf approved these changes

@j0k3r j0k3r Awaiting requested review from j0k3r

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace Ansible with Bash script
3 participants
@ngosang @Kdecherf @j0k3r