-
-
Notifications
You must be signed in to change notification settings - Fork 13k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
listmonk: 2.5.1 -> 3.0.0 #286523
listmonk: 2.5.1 -> 3.0.0 #286523
Conversation
@MarcelCoding you probably want to split this single commit into two: 1.) Updating the Package 2.) Fixing the Module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes a lot of things, I would like you to split the changes in multiple atomic commits with commit messages explaining why you are doing a particular change. Thank you.
7a1d4d7
to
4df7728
Compare
I've split the commits into the update and the module fix. I've also documented the motivation behind the changes in the frontend update. (commit message) Edit: I've tried testing the change, although now there is a problem with the hash of the lockfile, I'll notify you when I resolve it. |
4df7728
to
4bfcc65
Compare
That's not sufficient, the update contains much more than simply a update, please split more. |
But like how would I split it?
|
4bfcc65
to
cf120ad
Compare
This is a good time to move to |
listmonk switched their frontend toolchain to vite the existing past usage of yarn2nix did not work using the new toolchain setup. Therefore, I migrated the derivation to the "normal" yarn2nix.
I've fixed the broken hash and also moved the package into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me.
@@ -187,7 +187,11 @@ in { | |||
# Indeed, it will try to create all the folders and realize one of them already exist. | |||
# Therefore, we have to create it ourselves. | |||
''${pkgs.coreutils}/bin/mkdir -p "''${STATE_DIRECTORY}/listmonk/uploads"'' | |||
"${cfg.package}/bin/listmonk --config ${cfgFile} --idempotent --install --upgrade --yes" | |||
# setup database if not already done | |||
"${cfg.package}/bin/listmonk --config ${cfgFile} --idempotent --install --yes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use lib.getExe here
|
||
src = fetchFromGitHub { | ||
owner = "knadh"; | ||
repo = "listmonk"; | ||
rev = "v${version}"; | ||
sha256 = "sha256-gCnIblc83CmG1auvYYxqW/xBl6Oy1KHGkqSY/3yIm3I="; | ||
sha256 = "sha256-eNX+2ens+mz2V8ZBHtFFHDVbi64AAiiREElMjh67Dd8="; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use hash
Description of changes
https://github.com/knadh/listmonk/releases/tag/v3.0.0
EDIT:
istmonk switched their frontend toolchain to vite existing past usage of yarn2nix did not work using the new toolchain setup. Therefore, I migrated the derivation to the "normal" yarn2nix.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 馃憤 reaction to pull requests you find important.