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

plausible: 1.3.0 -> 1.4.0 #143345

Merged
merged 3 commits into from
Nov 8, 2021
Merged

plausible: 1.3.0 -> 1.4.0 #143345

merged 3 commits into from
Nov 8, 2021

Conversation

happysalada
Copy link
Contributor

@happysalada happysalada commented Oct 28, 2021

Motivation for this change

upstream update.

I have one question. I was thinking of using the recently made fetchYarnDeps to not need the giant yarn.nix file. This would require a manual change to the upstream package.json. Basically package.json has two dependencies using the version format file:path, and those wouldn't work with fetchYarnDeps. At the moment the upstream package.json version for those 2 files needs to be amended anyways (to change the path). I am considering just fixing the version (which can be found in the mix.lock file). The drawback is that this is a manual change that would be hard to automate. The benefit is that it enables the use of fetchYarnDeps. Happy to have feedback on this.

@yu-re-ka btw tried using fetchYarnDeps with an empty hash, and it returned an error. It would be cool if like other fetchers, if you supply an empty hash, it assumes the lib.fakeSha256.

This PR only updates the package, there are also changes to the modules that I am currently testing.

(detail, but I opened a PR upstream to add the version to the package.json to not have problem with yarn plausible/analytics#1416).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@happysalada
Copy link
Contributor Author

happysalada commented Oct 28, 2021

I've made 2 changes to the module.

  • run postgres init script by the postgres user so as not to require sudo
  • use loadcredentials for secrets.
  • grouped the start commands into one script (the loadCredentials are not available in ExecPreStart).

@happysalada happysalada force-pushed the update_plausible branch 3 times, most recently from a2ab77d to d1b9e65 Compare October 28, 2021 11:03
@happysalada
Copy link
Contributor Author

happysalada commented Oct 28, 2021

One other question I had was regarding the dependency on epmd.
I would like to add the explicit depedency in the module.
This means that next time you start the service, instead of trying to rely on the epmd started by plausible, it will rely on the global epmd. The problem with the epmd started by plausible is that when you restart the service, it becomes a zombie process.
Using the global epmd has one drawback though. For people already using the plausible service, they will have to kill the existing zombie epmd. It might be a little surprising. I'm thinking it's best to make this change when there aren't too many users yet. The other drawback is that the default epmd requires ipv6 enabled.

@yu-re-ka
Copy link
Contributor

Does yarn2nix work without changes to package.json?

@happysalada
Copy link
Contributor Author

@yu-re-ka yarn2nix says unsupported file:path, ignoring basically.
(in our case it's fine, because we give it the right path later, but it's a bit cheating).
I think the best solution in this case is just to change the package.json to get the real version numbers.

@yu-re-ka
Copy link
Contributor

We can change fetchYarnDeps to ignore file: packages as well if that helps

@happysalada
Copy link
Contributor Author

You"re right actually that would be the best solution.

@yu-re-ka
Copy link
Contributor

I added it to #143406, which touches the same logic as well

@happysalada happysalada marked this pull request as draft October 28, 2021 17:12
@happysalada
Copy link
Contributor Author

Testing this, it's still problematic with the loadcredentials part. I'm marking this as draft until I resolve it.

@happysalada happysalada force-pushed the update_plausible branch 2 times, most recently from d7049b0 to 0a67272 Compare October 30, 2021 09:02
@happysalada
Copy link
Contributor Author

hmmm, everything is working, however the admin user password seems to be different from the one supplied.
I want to debug but don't know how to get an environment with all the same environment variables and a tty.
It seems I should use systemd-run, if anybody knows how to use it, I am interested.
with elixir I can get a remote console into the running program to check what went wrong. I just don't know how to use systemd-run. I'll have a look in more details tomorrow.

@happysalada happysalada force-pushed the update_plausible branch 3 times, most recently from 7d130d9 to b604f72 Compare October 30, 2021 12:58
@happysalada happysalada marked this pull request as ready for review October 30, 2021 12:58
@happysalada
Copy link
Contributor Author

@Ma27 this is ready for review. I was going insane, I just had a newline in my password file. Well live and learn.
No rush on this, let me know if you have any feedback.

@happysalada
Copy link
Contributor Author

I think I have found a nice way to provide users with a way to connect to a running plausible node to do some debugging, however, I'll try that in a subsequent PR (will test the change to fetchYarnDeps as well if you are open to it.)

};
};
}
(mkIf cfg.database.postgres.setup {
# `plausible' requires the `citext'-extension.
plausible-postgres = {
after = [ "postgresql.service" ];
bindsTo = [ "postgresql.service" ];
requiredBy = [ "plausible.service" ];
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, whyis this not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad for not detailing a bit more.
bindsTo is just a strong form of requires, where if the required dependency stops, then the dependency stops as well. It doesn't really make sense for a setup script that just runs for several seconds (IMO). Open to discussion of course.
requiredBy, is usually never used in the configuration settings directly and is just the symetric of required.
https://www.freedesktop.org/software/systemd/man/systemd.unit.html#Mapping%20of%20unit%20properties%20to%20their%20inverses
(from the Note section).
Perhaps I read that in the wrong way. Open for discussions as well.

./ecto_sql-fix.patch
./plausible-Bump-clickhouse_ecto-dependency-to-be-compatible-with-ecto-3.6.patch
];
sha256 = "sha256-ZQfrTxsLzCWFf3vabOk0vyHWZLw69GJovm3vR+7UbMY=";
};
Copy link
Member

Choose a reason for hiding this comment

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

Does this still work fine with the update script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated the script to make sure it works with the new version.

@ofborg ofborg bot requested a review from Ma27 November 1, 2021 00:50
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

The test doesn't evaluate anymore. First of all, you have to use runCommand rather than runCommandNoCC (fixed in a yarn2nix on nixpkgs master IIRC) and when you do this, it still fails like this:

error: undefined variable 'plausibleConsole'

       at /home/ma27/Projects/nixpkgs/nixos/modules/services/web-apps/plausible.nix:166:36:

          165|
          166|     environment.systemPackages = [ plausibleConsole ];
             |                                    ^
          167|
(use '--show-trace' to show detailed location information)

plausible: service fixes, remove console attempt

plausible: fix yarn.nix call
plausible: fix yarn.nix call
@happysalada
Copy link
Contributor Author

@Ma27 My bad about missing those. I've committed fixes.

@ofborg ofborg bot requested a review from Ma27 November 6, 2021 13:26
@yu-re-ka
Copy link
Contributor

yu-re-ka commented Nov 8, 2021

Can we put the package update into a seperate PR, so I can review and merge them independently of the ongoing discussions regarding module changes?

@happysalada
Copy link
Contributor Author

I would like to, but there are some updates needed to the module as well to make the new version work. Some environment variable names that changed. I realise though that this PR was a bit large, and should have been broken down a little more before hand.

@happysalada
Copy link
Contributor Author

I'll tell you what, if your PR is ready before this is approved, then just merge it, I'll do the rebase afterwards, no worries.

@Ma27
Copy link
Member

Ma27 commented Nov 8, 2021

It's on my todo list to review & merge this week. If that's good enough for @yu-re-ka, then we can leave it as-is :)

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Nov 8, 2021

I would like to, but there are some updates needed to the module as well to make the new version work

I see, then it has to be one big PR, which is also fine.

It's on my todo list to review & merge this week

Definitely good enough :)
I'm also running the tests now so maybe you will have less work once you get around to reviewing it.

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Nov 8, 2021

✅ Package builds, test passes on x86_64-linux

@Ma27 Ma27 merged commit 25e6a0a into NixOS:master Nov 8, 2021
@happysalada happysalada deleted the update_plausible branch November 9, 2021 10:50
@nh2
Copy link
Contributor

nh2 commented Jan 13, 2022

The problem with the epmd started by plausible is that when you restart the service, it becomes a zombie process.

@happysalada Related question: #130297 (comment)

@happysalada
Copy link
Contributor Author

So just to comment further. When you update from the old version to the new one, you will need to kill one epmd process that goes zombie. However on subsequent restarts, there will be no problem.
Let me know if that doesn't answer your question though.

@nh2
Copy link
Contributor

nh2 commented Jan 13, 2022

@happysalada That doesn't quite answer it -- I'll ask in the linked thread.

@RaitoBezarius
Copy link
Member

This broke secret handling for Plausible and it seems this was never tested/debugged properly…

LoadCredential do not create any environment variables and require read from the filesystem.

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

Successfully merging this pull request may close these issues.

5 participants