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

restya-board: Restya board config fix #74763

Merged
merged 1 commit into from Dec 23, 2019
Merged

Conversation

@nek0
Copy link
Contributor

nek0 commented Dec 1, 2019

Motivation for this change

Configuration options for restya-board were broken.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 2, 2019

@nek0 the version of restya-board we have in nixpkgs is over 2 years old and 7 releases behind. For a web application this is deeply concerning given the security implications. Do you have any interest and ability to look into bringing this package up to date?

Copy link
Contributor

aanderse left a comment

You should leave the option name as passwordFile and correct the sed call only. No more password options which pollute the world readable nix store with secrets. Using file_get_contents will allow the program to access the passwordFile at runtime and keep the value secret+secure.

If you require any clarification on what I mean by this please do not hesitate to ask and I can provide more details.

@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 2, 2019

@aanderse I think I can update the version of this package, I'm just unsure whether this requires a seperate pull request.
Concerning the password option, I will change that as you suggested.

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 2, 2019

@nek0 that is great to hear! Yes, a separate pull request would be great. Thank you!

@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 3, 2019

For newer restya-board version see pull request #74888.

@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 5, 2019

@aanderse Can you have a look at how I invoke the migration scripts? I don't particularly like my solution, but given the constraints I encountered it was the only option I found viable.

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 5, 2019

Oh the program forces the sysadmin to manually manage migrations? The program has no option to do it for you?

@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 5, 2019

Unfortunately no. Restya-board seems to have no knowledge of its own installed version and I found no possibility to extract that information out of the nix store.
Here's the section on upgrading in restya-board's readme, which is basically just links to the sql-files to apply.

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 5, 2019

That is unfortunate. In that case I worry this might be a bit too fragile to allow NixOS to run the migrations for the user... If some user skips a NixOS version and we bump previousVersion the user would be in trouble. If the user installs a custom version they would be in trouble. Other unforeseen issues, potentially.

Maybe best to add a note to the documentation? What does restya-board do if you access the application without having run the most recent migration(s)?

Given the problems associated with running database migrations in the module I'd ask that @uvNikita, @c0bw3b, and @ryantm reconsider the conclusion of requiring NixOS to run database migrations agreed upon in #49618 and potentially provide some feedback on this PR.

@c0bw3b

This comment has been minimized.

Copy link
Contributor

c0bw3b commented Dec 5, 2019

Given the risks for failures and if upstream expects the migration to be an admin action, I'm ✔️ on not doing it automatically through the service module.
Provided we have a clear message about it in the release notes.

@c0bw3b c0bw3b mentioned this pull request Dec 5, 2019
2 of 9 tasks complete
@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 6, 2019

Thanks for your feedback @c0bw3b.

@nek0 can you please:

  • drop your last commit
  • squash the 2nd, 3rd, and 4th commits
  • add some notes about database migrations being the responsibility of the sysadmin, including some links to upstream documentation

Thanks for all your hard work on this!

@nek0 nek0 force-pushed the nek0:restya-board-config-fix branch from 6029167 to 3cf1554 Dec 6, 2019
@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 6, 2019

Reverting and squashing done. Where do I put the notes on the upgrade process best, in the module, the package's meta-information or somewhere completely different?

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 15, 2019

@nek0 possibly worth mention in the release notes (nixpkgs/nixos/doc/manual/release-notes/), but maybe even in the package or service definition as well. Seems like a pretty important thing we really want to make sure users don't miss. That make sense to you too @c0bw3b?

@c0bw3b

This comment has been minimized.

Copy link
Contributor

c0bw3b commented Dec 15, 2019

Yes it's sensible : a note in the longDescription of the package to warn nixpkgs users that updates may need manual actions + a message in the 20.03 release note for the module users from NixOS

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 22, 2019

@nek0 seems like the only outstanding piece here is notes in the meta.longDescription and release notes. Are you able to address this?

@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 22, 2019

Yes. I can do that. Sorry for the delay. I had some RL issues...

@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 22, 2019

OK. I have created said notes in de65dc3, which belongs to #74888.

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 22, 2019

@nek0 no problem at all. Thanks for looking at it!

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Dec 23, 2019

@nek0 everything is sorted out on #74888 so there is no reason to hold this PR up anymore. Can you please squash this PR into a single commit and we'll merge it? Thank you for all your hard work on restya-board, you're great! 🎉

@nek0 nek0 force-pushed the nek0:restya-board-config-fix branch from b9ea47b to acd1240 Dec 23, 2019
@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 23, 2019

ok. squashed.

@c0bw3b
c0bw3b approved these changes Dec 23, 2019
@aanderse aanderse merged commit 133a5c3 into NixOS:master Dec 23, 2019
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
restya-board on aarch64-linux Success
Details
restya-board on x86_64-linux Success
Details
@nek0

This comment has been minimized.

Copy link
Contributor Author

nek0 commented Dec 23, 2019

I'm sorry, but it seems I made a mistake in the configuration file. I'm working on a fix right now.

@nek0 nek0 mentioned this pull request Dec 23, 2019
3 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.