-
Notifications
You must be signed in to change notification settings - Fork 8
nix build system part 3 (don't merge me yet) #1252
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Frr does not like these options sadly. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Remove un-needed build.rs files and adjust extant build.rs files to work with nix build system. Note that network access is not possible within a nix build. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Needed for gdbserver in debug container Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
| .replace( | ||
| "idle_timeout: Option<String>", | ||
| "idle_timeout: Option<kube_core::duration::Duration>", | ||
| "idle_timeout: Option<std::time::Duration>", |
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.
Are you sure that this is the right fix? It has parsing implications.
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.
Also, this type of fix should really be in its own PR, it has nothing to do with the build system.
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.
The change above will BREAK the integration with k8s and should be reverted.
| // This should get both vtep_mtu and plain mtu | ||
| .replace("mtu: Option<i32>", "mtu: Option<u32>") | ||
| .replace("vni: Option<i32>", "vni: Option<u32>") | ||
| .replace("workers: Option<i64>", "workers: Option<u8>") // Gateway Go code says this is a u8 |
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.
Was there an error where the actual thing was workers: Option<i64> instead of workers: Option<u64> in the original source? In the future, this type of refactor/fix needs to be its own commit, or maybe even its own PR. It has nothing to do with the build system changes. Please resist the urge to change stuff that isn't related to the PR. You can always make a note and come back to it in a separate PR or commit with explanation of why the change was made.
Anyway, no need to fix it here unless the change is wrong.
| fetch_crd(&agent_crd_url) | ||
| } else { | ||
| panic!("No CRD path or URL is set in env file"); | ||
| let agent_crd_contents = { |
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.
Will this still allow manual builds inside of nix shell? I think it will but I want to be sure as that use case must still work for debugging and testing. We should also add docs on how to execute some of the normal work flows with the new build system.
| max-jobs = 4 | ||
| experimental-features = nix-command | ||
| substituters = https://dnoland-test-cache.cachix.org/ https://cache.nixos.org https://cache.nixos.org/ | ||
| trusted-public-keys = dnoland-test-cache.cachix.org-1:AsVLS3c7NGIJbUHW5xOlK69zPEzY5jNF+ax0jd1LKUA= cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY= |
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 will be changed right?
ignore the last two commits, (they are here to manage rollover to the new build system)