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

New Script: silverbullet #659

Merged
merged 9 commits into from
Dec 20, 2024
Merged

Conversation

dsiebel
Copy link
Contributor

@dsiebel dsiebel commented Dec 3, 2024

Description

I was looking into Silverbullet today and while installing it in a debian LXC I created this install and update script. Since Silverbullet was already requested (#127) and it's already on the backlog, I figured: why not.

Fixes #127

Type of change

Please check the relevant option(s):

  • Bug fix (non-breaking change that resolves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (a fix or feature that would cause existing functionality to change unexpectedly)
  • New script (a fully functional and thoroughly tested script or set of scripts.)

Prerequisites

The following efforts must be made for the PR to be considered. Please check when completed:

  • Self-review performed (I have reviewed my code, ensuring it follows established patterns and conventions)
  • Testing performed (I have tested my changes, ensuring everything works as expected)
  • Documentation updated (I have updated any relevant documentation)

Additional Information (optional)

I intentionally decided against using the silverbullet upgrade command, since this would require installing and running the entire deno build. Using pre-built binary just feels better for production use.

Ref: https://silverbullet.md/Install/Deno#Upgrading

Related Pull Requests / Discussions

If there are other pull requests or discussions related to this change, please link them here:

  • Related PR #

@dsiebel dsiebel requested a review from a team as a code owner December 3, 2024 15:22
@github-actions github-actions bot added new script A change that adds a new script website A change to the website labels Dec 3, 2024
@MickLesk
Copy link
Member

MickLesk commented Dec 3, 2024

It's not worth checking every line for now, there are so many errors and things that don't fit the conventions of our system.

Please check correspondence objects such as kimai, snipe-it, komga, vikunja, bookstack etc.

@dsiebel
Copy link
Contributor Author

dsiebel commented Dec 3, 2024

It's not worth checking every line for now, there are so many errors and things that don't fit the conventions of our system.

Please check correspondence objects such as kimai, snipe-it, komga, vikunja, bookstack etc.

I would really appreciate some pointers then. I used the scripts to install and update Silverbullet on my running Debian LXC, so I am wondering what errors there are.

To be quite frank, it is impossible at the moment to find "the right way" to do things, and I get the feeling it keeps changing with every PR I create..
I looked at traefik, trilium, debian and zoraxy, which I recently updated, when creating the scripts.

I did add some improvements (or so I thought), like checking the service is actually running after install and update. Is this what you are referring to?

@dsiebel
Copy link
Contributor Author

dsiebel commented Dec 3, 2024

To visualize my point, this is a side-by-side comparison between the update scripts of vikunja and the newly added silverbullet. The boilerplate code is all the same, the things that are different are because it's a different app that's being installed.

image

ct/silverbullet.sh Outdated Show resolved Hide resolved
ct/silverbullet.sh Outdated Show resolved Hide resolved
ct/silverbullet.sh Outdated Show resolved Hide resolved
ct/silverbullet.sh Outdated Show resolved Hide resolved
ct/silverbullet.sh Outdated Show resolved Hide resolved
install/silverbullet-install.sh Outdated Show resolved Hide resolved
install/silverbullet-install.sh Outdated Show resolved Hide resolved
install/silverbullet-install.sh Outdated Show resolved Hide resolved
install/silverbullet-install.sh Outdated Show resolved Hide resolved
json/silverbullet.json Outdated Show resolved Hide resolved
@MickLesk
Copy link
Member

MickLesk commented Dec 3, 2024

Short review only with the mobile phone. We don't do the whole thing with services etc. like this. In general, take a look at current scripts and not the ones you mentioned above, which have not been revised for ages.

@dsiebel
Copy link
Contributor Author

dsiebel commented Dec 3, 2024

Thanks for the review! Will address asap.

ct/silverbullet.sh Outdated Show resolved Hide resolved
ct/silverbullet.sh Outdated Show resolved Hide resolved
@dsiebel
Copy link
Contributor Author

dsiebel commented Dec 16, 2024

I noticed the project card for silverbullet hasn't moved to "In PR" yet. Can someone take care of it for transparency?

Copy link
Member

@michelroegl-brunner michelroegl-brunner left a comment

Choose a reason for hiding this comment

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

LGTM

@MickLesk MickLesk merged commit 7f86418 into community-scripts:main Dec 20, 2024
3 checks passed
@MickLesk MickLesk changed the title new script: silverbullet New Script: silverbullet Dec 20, 2024
@dsiebel dsiebel deleted the silverbullet branch December 20, 2024 10:03
dsiebel added a commit to dsiebel/ProxmoxVE that referenced this pull request Dec 27, 2024
* new script: silverbullet

* update copyright and author information

* address code review comments and suggestions

* fix: overwrite existing binary on unzip

* address remaining review comments

* add documenation link

* update header and footer to new standard

* little fixes

* little fixes

---------

Co-authored-by: CanbiZ <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new script A change that adds a new script website A change to the website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants