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

netbox: 3.3.9 -> 3.4.7, netbox_3_3: init at 3.3.10, RFC42-style options, more tests #206983

Merged
merged 13 commits into from
Apr 5, 2023

Conversation

minijackson
Copy link
Member

@minijackson minijackson commented Dec 20, 2022

Description of changes

Also added RFC42-style options, which introduces a new pythonVars format which might be interesting for other Django apps.

Also extended the test, since we missed the Django 4.1 incompatibility / LDAP integration issue earlier.

Added myself as maintainer, since I will be probably using it for quite some time ^^

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, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@RaitoBezarius @n0emis

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 20, 2022
@ofborg ofborg bot requested review from n0emis and RaitoBezarius December 20, 2022 09:47
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels Dec 20, 2022
@RaitoBezarius
Copy link
Member

Thank you for this amazing change, I wanted to revisit our options since a long time, thank you for doing it. I hope to take a look to this in the next week.

@RaitoBezarius RaitoBezarius changed the title Netbox 3.3.9 -> 3.4.1 netbox: 3.3.9 -> 3.4.1, RFC42-style options, more tests Dec 21, 2022
@minijackson minijackson changed the title netbox: 3.3.9 -> 3.4.1, RFC42-style options, more tests netbox: 3.3.9 -> 3.4.2, RFC42-style options, more tests Jan 4, 2023
@centromere
Copy link
Member

3.4.3 has been released.

@RaitoBezarius, are you able to give this a review?

@minijackson minijackson changed the title netbox: 3.3.9 -> 3.4.2, RFC42-style options, more tests netbox: 3.3.9 -> 3.4.3, RFC42-style options, more tests Jan 21, 2023
@minijackson
Copy link
Member Author

Upgraded to 3.4.3, and rebased due to release note conflicts

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Well, all of this is very high quality. :)
My only worries are the following:

  • should not we introduce a state version mechanism?
  • should we introduce a flag to disable automatic migrations? (to prevent any dataloss)


format = "other";

src = fetchFromGitHub {
owner = "netbox-community";
repo = pname;
rev = "refs/tags/v${version}";
sha256 = "sha256-KhnxD5pjlEIgISl4RMbhLCDwgUDfGFRi88ZcP1ndMhI=";
sha256 = "sha256-M/CbnlnRaWuwcJJofY5mPpVTl9zYrApsXqV9hilNyvw=";
};

patches = [
# Allow setting the STATIC_ROOT from within the configuration and setting a custom redis URL
./config.patch
Copy link
Member

Choose a reason for hiding this comment

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

Did we try to upstream this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, but I think we should

@RaitoBezarius
Copy link
Member

@minijackson Can I do something to help you on this PR? (also, can we bump it to the latest version?)

@minijackson
Copy link
Member Author

@minijackson Can I do something to help you on this PR? (also, can we bump it to the latest version?)

On it! I just had to have some time to do it.

@RaitoBezarius RaitoBezarius changed the title netbox: 3.3.9 -> 3.4.3, RFC42-style options, more tests netbox: 3.3.9 -> 3.4.5, RFC42-style options, more tests Mar 12, 2023
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

The only problem I "still have" regarding this PR is the automatic migrations.
But it is hard to take a decision.
Well, we have explicit release notes about it, so this is only a weak blocker for me.
We should definitely ask what is upstream opinion on this now and if we need to introduce proper stateVersion and graceful upgrades.

@RaitoBezarius
Copy link
Member

Result of nixpkgs-review pr 206983 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
1 package failed to build:
  • netbox

@RaitoBezarius
Copy link
Member

Redis is failing to build for me:

> !!! WARNING The following tests failed:
       >
       > *** [err]: AOF will open a temporary INCR AOF to accumulate data until the first AOFRW success when AOF is dynamically enabled in tests/integration/aof-multi-part.tcl
       > Expected 'file appendonly.aof.2.base.rdb seq 2 type b' to be equal to 'file appendonly.aof.1.base.rdb seq 1 type b' (context: type source line 125 file /build/redis-7.0.9/tests/support/aofmanifest.tcl cmd {assert_equal [lindex $lines $i] [lindex $content $i]} proc ::assert_aof_manifest_content level 2)
       > Cleanup: may take some time... OK
       For full logs, run 'nix log /nix/store/68a2mzcsidqb8h4kz153c31c0drwj7hm-redis-7.0.9.drv'.
error: builder for '/nix/store/68a2mzcsidqb8h4kz153c31c0drwj7hm-redis-7.0.9.drv' failed with exit code 1
error: 1 dependencies of derivation '/nix/store/bkzshfakd85403dj97dh9c7ark208zm7-python3.10-django-redis-5.2.0.drv' failed to build
error: 1 dependencies of derivation '/nix/store/190g51x83kzjy362yaz7k8z8vhzj513j-netbox-3.4.5.drv' failed to build
error: 1 dependencies of derivation '/nix/store/fap8zc20jsq341yccf2f8ncywdd99gfm-review-shell.drv' failed to build

@minijackson
Copy link
Member Author

That's weird, I just rebased, and ran nix build '.#nixosTests.netbox'. I had to rebuild the kernel and redis, but it built without issues...

@KiruyaMomochi
Copy link
Contributor

KiruyaMomochi commented Mar 22, 2023

When running the latest nixpkgs with patch from this PR, I can't start netbox successfully.

Mar 22 08:24:33 carmina systemd[1]: Starting NetBox migrations...
Mar 22 08:24:35 carmina netbox[74931]: Operations to perform:
Mar 22 08:24:35 carmina netbox[74931]:   Apply all migrations: admin, auth, circuits, contenttypes, dcim, django_rq, extras, ipam, sessions, social_django, taggit, tenancy, users, virtualization, wireless
Mar 22 08:24:36 carmina netbox[74931]: Running migrations:
Mar 22 08:24:36 carmina netbox[74931]:   No migrations to apply.
Mar 22 08:24:36 carmina netbox[74931]:   Your models in app(s): 'social_django' have changes that are not yet reflected in a migration, and so won't be applied.
Mar 22 08:24:36 carmina netbox[74931]:   Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.
Mar 22 08:24:36 carmina systemd[1]: netbox-migration.service: Deactivated successfully.
Mar 22 08:24:36 carmina systemd[1]: Finished NetBox migrations.
Mar 22 08:24:36 carmina systemd[1]: netbox-migration.service: Consumed 2.723s CPU time, no IP traffic.

I guess the error is introduced by the 5.1.0 update of social-auth-app-django. We may need to pin an older version in the package file.

I have tried following NixOS wiki to override social-auth-app-django in an overlay. However, neither overriding netbox or just override python3 package works for me.

Is this because python3 has already be overriden by this package?

https://github.com/NixOS/nixpkgs/blob/297c4f66bafbe65e8b27fe914c9aaa625bce2a18/pkgs/servers/web-apps/netbox/default.nix#L11-L15

@RaitoBezarius RaitoBezarius mentioned this pull request Mar 25, 2023
12 tasks
@stuebinm
Copy link
Contributor

I have tried following NixOS wiki to override social-auth-app-django in an overlay. However, neither overriding netbox or just override python3 package works for me.

Is this because python3 has already be overriden by this package?

https://github.com/NixOS/nixpkgs/blob/297c4f66bafbe65e8b27fe914c9aaa625bce2a18/pkgs/servers/web-apps/netbox/default.nix#L11-L15

It is, this is a limitation of how python's packageOverrides work in nixpkgs. I've run into this issue before, and just opened #223268 with a workaround to allow overrides to work as expected.

Until then, an easy but hacky workaround (without changing nixpkgs) is to also put django = super.django_4 into your own package overrides, and then disable python's overriding functionality entirely so the snippet you linked can't change it back (e.g. by doing let python3 = (python3.override { (your package overrides) }) // { override = _: python3 } in ...).

@RaitoBezarius
Copy link
Member

3.4.7 is out, can I do something to help you get to the finish line here @minijackson ? I can take over and push the last changes if needed.

@RaitoBezarius
Copy link
Member

@minijackson friendly ping so that I can take over if you're too busy right now :)

@minijackson
Copy link
Member Author

@RaitoBezarius sure, go ahead. Sorry for the delay...

@RaitoBezarius
Copy link
Member

@minijackson @SuperSandro2000 please could you give a review to this?

@minijackson
Copy link
Member Author

Seems good, thanks! (Can't make this a GitHub approval, since I'm the original author of the PR)

All tests pass on my machine:

nix build -f ./nixpkgs "netbox.tests.netbox"
NIXPKGS_ALLOW_INSECURE=1 nix build -f ./nixpkgs "netbox_3_3.tests.netbox"

Result of nixpkgs-review pr 206983 run on x86_64-linux 1

1 package marked as broken and skipped:
  • netbox_3_3
2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
1 package built:
  • netbox

@RaitoBezarius RaitoBezarius changed the title netbox: 3.3.9 -> 3.4.5, RFC42-style options, more tests netbox: 3.3.9 -> 3.4.7, netbox_3_3: init at 3.3.10, RFC42-style options, more tests Apr 5, 2023
@RaitoBezarius RaitoBezarius merged commit a6bc6ed into NixOS:master Apr 5, 2023
@RaitoBezarius
Copy link
Member

For the next time, let's try to do smaller PRs and smaller scale changes to get easier merges. :)
Thank you to everyone involved!

@minijackson minijackson deleted the netbox-3.4.1 branch April 26, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants