Fig regression of peach-dyndns-updater #63

notplants · 2022-01-11T23:10:18Z

notplants commented

2022-01-11 23:10:18 +00:00

This PR fixes a regression of peach-dyndns-updater. I still don't totally understand the cause of the regression (maybe upgrade of rust?), so I've just changed to use nsupdate in a slightly different way which is working now (writing the arguments to a file, and then calling nsupdate passing the path to the file as an argument, instead of instantiating a child process and passing the arguments by writing to stdin for the child).

I've also

added the ability to configure the URL for the peach-dyndns-server (to allow for more easily testing a development server... important for fixing stuff when this is live),
added some brief documentation of the configs
fixed some clippy warnings

I've tested on the pi that this is working.

This PR fixes a regression of peach-dyndns-updater. I still don't totally understand the cause of the regression (maybe upgrade of rust?), so I've just changed to use nsupdate in a slightly different way which is working now (writing the arguments to a file, and then calling nsupdate passing the path to the file as an argument, instead of instantiating a child process and passing the arguments by writing to stdin for the child). I've also - added the ability to configure the URL for the peach-dyndns-server (to allow for more easily testing a development server... important for fixing stuff when this is live), - added some brief documentation of the configs - fixed some clippy warnings I've tested on the pi that this is working.

notplants added 5 commits 2022-01-11 23:10:18 +00:00

29cc40be48 Fix setup of nsupdate

b8f394b901 Debugging dyndns

c75608fb1a Merge branch 'main' of https://git.coopcloud.tech/PeachCloud/peach-workspace into main3

1ea0ea2ed1 Fix regression of peach-dyndns-updater

171d051710 Fix clippy warmings

notplants added 1 commit 2022-01-11 23:10:42 +00:00

bdfbd7057f Remove commented out code

notplants requested review from glyph 2022-01-11 23:10:59 +00:00

glyph added 3 commits 2022-01-12 08:59:28 +00:00

4adf5547c9 formatting

4a27892ab6 idiomatic paths and result type for checking new dns address

c3fbc5cd73 try operator for dyns dns domain check

glyph commented

2022-01-12 09:04:19 +00:00

@notplants

I've just changed to use nsupdate in a slightly different way which is working now (writing the arguments to a file, and then calling nsupdate passing the path to the file as an argument, instead of instantiating a child process and passing the arguments by writing to stdin for the child).

Nice, that sounds like a good approach to me.

Great PR! Very happy to have this in working order :) Now we'll have two ways of testing inter-peach replication (DYN DNS & Tor hidden service).

I added a couple commits:

Using idiomatic paths for config_manager function calls
Ran cargo fmt to tidy-up a couple small formatting issues
Replaced the .unwrap() with Result<bool, PeachError> + ? for the function which checks if the dyn dns domain is new
- Added a ? operator for the function call in peach-web
- This unwrap() bit me when experimenting with rouille and maud (server crash when visiting the DNS settings page) - figured I'd take care of it while my memory was fresh

@notplants > I've just changed to use nsupdate in a slightly different way which is working now (writing the arguments to a file, and then calling nsupdate passing the path to the file as an argument, instead of instantiating a child process and passing the arguments by writing to stdin for the child). Nice, that sounds like a good approach to me. Great PR! Very happy to have this in working order :) Now we'll have two ways of testing inter-peach replication (DYN DNS & Tor hidden service). I added a couple commits: - Using idiomatic paths for `config_manager` function calls - Ran `cargo fmt` to tidy-up a couple small formatting issues - Replaced the `.unwrap()` with `Result<bool, PeachError>` + `?` for the function which checks if the dyn dns domain is new - Added a `?` operator for the function call in `peach-web` - This `unwrap()` bit me when experimenting with `rouille` and `maud` (server crash when visiting the DNS settings page) - figured I'd take care of it while my memory was fresh

glyph added 14 commits 2022-01-12 09:51:13 +00:00

a05e67c22f bump patch version

4b0b2626a4 update code examples

58f2ddde05 replace String with str and remove unnecessary structs

731bc1958b Merge pull request 'Update network args and remove structs' (#60 ) from update_network_args into main

Reviewed-on: #60

a0e80fcda7 add deps for network and stats

3ab3e65eb7 replace stats rpc client calls with direct calls to peach_stats

567b0bbc2a replace network rpc client calls with direct calls to peach_network

446927f587 replace outdated crypto crate

17d52c771f Merge branch 'main' into direct_call_net_stats

Merge crypto library update for peach-lib.

49ad74595c cleanup use paths and leave network_ping note

57ed0ab66a add fullstop to docs sentence

699f2b13c9 merge update_network_args branch

037e5c34b6 merge replace_rust_crypto branch

a5415aad99 merge direct_call_net_stats branch

glyph commented

2022-01-12 09:58:18 +00:00

@notplants

Ah, this turned into a nightmare. I noticed there was a conflict for Cargo.toml for your PR and then realised that the branch you had forked from to create fix_regression was not up to date with main. Ironically, the fix_regression PR would have introduced multiple regressions.

I think I have merged all the outstanding changes: PR #60, PR #61, & PR #62.

I also discovered an issue with compilation at the workspace-level. Because the miniserde_support feature flag is enabled for peach-lib in one crate and the serde_support flag is enabled in another crate, cargo fails with errors like this:

error[E0119]: conflicting implementations of trait miniserde::Serializefor typestats::MemStat``

This is annoying. Here's a small SO post about it:

https://stackoverflow.com/questions/64200237/multiple-versions-of-local-dependency-in-cargo-workspace-with-different-features

I'll figure out a solution later.

@notplants Ah, this turned into a nightmare. I noticed there was a conflict for `Cargo.toml` for your PR and then realised that the branch you had forked from to create `fix_regression` was not up to date with `main`. Ironically, the `fix_regression` PR would have introduced multiple regressions. I think I have merged all the outstanding changes: [PR #60](https://git.coopcloud.tech/PeachCloud/peach-workspace/pulls/60), [PR #61](https://git.coopcloud.tech/PeachCloud/peach-workspace/pulls/61), & [PR #62](https://git.coopcloud.tech/PeachCloud/peach-workspace/pulls/62). I also discovered an issue with compilation at the workspace-level. Because the `miniserde_support` feature flag is enabled for `peach-lib` in one crate and the `serde_support` flag is enabled in another crate, `cargo` fails with errors like this: `error[E0119]: conflicting implementations of trait `miniserde::Serialize` for type `stats::MemStat`` This is annoying. Here's a small SO post about it: https://stackoverflow.com/questions/64200237/multiple-versions-of-local-dependency-in-cargo-workspace-with-different-features I'll figure out a solution later.

glyph merged commit cd1fb697f7 into main

2022-01-12 09:58:46 +00:00

glyph referenced this issue from a commit

2022-01-12 09:58:47 +00:00

Merge pull request 'Fig regression of peach-dyndns-updater' (#63) from fix-regression into main

glyph commented

2022-01-12 09:59:06 +00:00

Note: I did not delete the branch, just in case we have to revert the merge.

glyph referenced this pull request

2022-01-12 10:24:49 +00:00

Replace miniserde_support with serde_support for peach-jsonrpc-server #64

notplants commented

2022-01-12 15:27:36 +00:00

@glyph with your original fixing commits, thanks,

good to get rid of the unwrap
fix imports
... I was less familiar with those things in the spring when I was working on this and glad to feel more comfortable now

don't totally understand the merge conflict issue you ran into, but sounds like a mess and glad you got it sorted

@glyph with your original fixing commits, thanks, - good to get rid of the unwrap - fix imports ... I was less familiar with those things in the spring when I was working on this and glad to feel more comfortable now don't totally understand the merge conflict issue you ran into, but sounds like a mess and glad you got it sorted

❤️ 1

glyph commented

2022-01-12 15:32:54 +00:00

@notplants

It happens to me sometimes when I work on a branch without merging latest changes from main (so my branch then ends up being behind in some ways). I think in this case the particular changes you didn't have on your branch were from the PR which updated the crypto in peach-lib (when we switched to using the sha3 crate). So then there was a conflict in the Cargo.toml of peach-lib.

I figured I'd just go ahead and make the commits rather than asking you to make changes. That also makes it easier for you to get straight to work on other tasks whenever you start your day :)

@notplants It happens to me sometimes when I work on a branch without merging latest changes from `main` (so my branch then ends up being behind in some ways). I think in this case the particular changes you didn't have on your branch were from the PR which updated the crypto in `peach-lib` (when we switched to using the `sha3` crate). So then there was a conflict in the `Cargo.toml` of `peach-lib`. I figured I'd just go ahead and make the commits rather than asking you to make changes. That also makes it easier for you to get straight to work on other tasks whenever you start your day :)

notplants commented

2022-01-12 15:36:26 +00:00

I think I understand, that something about the Cargo.toml from the old branch + what I added, cauased a conflict, so it couldn't just straight-forward merge in the commits it was still missing.

and cool yes I have no problem at all with less polishing commits to do :)

I think I understand, that something about the Cargo.toml from the old branch + what I added, cauased a conflict, so it couldn't just straight-forward merge in the commits it was still missing. and cool yes I have no problem at all with less polishing commits to do :)

notplants referenced this pull request

2022-01-12 15:38:41 +00:00

Merged Elsewhere: Fix setup of nsupdate. Add installation of dnsutils. Dynamically get dynamic dns server domain. #59

Sign in to join this conversation.

No reviewers