PeachCloud
/
peach-workspace
3
2
Fork 1
Code Issues 17 Pull Requests Projects Releases Wiki Activity

Wide range of web improvements #70

Merged
glyph merged 18 commits from web_improvements into main 2022-01-17 09:35:30 +00:00
Conversation 6 Commits 18 Files Changed 27 +991 -1427
glyph commented 2022-01-14 13:46:30 +00:00
Owner
Copy Link

In the name of expedience, I'm breaking my personal rule about not making large PRs.

Changes

This PR introduces a number of changes / improvements / removals which were prompted by my experiment to convert peach-web to using rouille and maud.

  • Remove JSON API (incl. tests), JavaScript and noscript template snippet
  • Remove unneeded dependencies
  • Replace snafu with a custom error implementation
  • Replace RPC client calls with direct calls to peach-stats and peach-network
  • Add an env_var checker to toggle standalone mode
    • Mount routes accordingly in src/router.rs
  • Define static variables for WLAN_IFACE and AP_IFACE
  • Remove unnecessary context object structs (replaced with tera::Context type)
    • Particularly for templates which only need back, title and flash message data
  • Move context structs and builders to a dedicated directory
    • This is how it was originally; I realised I much prefer the separation (keeps src/routes files much quieter and cleared in my experience)
  • Fixed broken paths

Tests are passing (DISABLE_ROCKET_AUTH=true PEACH_STANDALONE_MODE=false cargo test).

There will undoubtedly be some bugs that I haven't caught but I think this PR gets us to a solid point of being able to focus purely on standalone mode features and polish in preparation for release.

I'm very happy that this PR effectively reduces the codebase by 500 lines.

CC: @notplants (I don't expect you to do a thorough review but hopefully the summary above serves to update you on the important changes).

In the name of expedience, I'm breaking my personal rule about not making large PRs. ----- ## Changes This PR introduces a number of changes / improvements / removals which were prompted by my experiment to convert peach-web to using `rouille` and `maud`. - Remove JSON API (incl. tests), JavaScript and `noscript` template snippet - Remove unneeded dependencies - Replace `snafu` with a custom error implementation - Replace RPC client calls with direct calls to `peach-stats` and `peach-network` - Add an env_var checker to toggle standalone mode - Mount routes accordingly in `src/router.rs` - Define static variables for `WLAN_IFACE` and `AP_IFACE` - Remove unnecessary context object `struct`s (replaced with `tera::Context` type) - Particularly for templates which only need `back`, `title` and flash message data - Move context `struct`s and builders to a dedicated directory - This is how it was originally; I realised I much prefer the separation (keeps `src/routes` files much quieter and cleared in my experience) - Fixed broken paths Tests are passing (`DISABLE_ROCKET_AUTH=true PEACH_STANDALONE_MODE=false cargo test`). There will undoubtedly be some bugs that I haven't caught but I think this PR gets us to a solid point of being able to focus purely on standalone mode features and polish in preparation for release. I'm very happy that this PR effectively reduces the codebase by 500 lines. CC: @notplants (I don't expect you to do a thorough review but hopefully the summary above serves to update you on the important changes).
glyph added the
refactor
peach-web
 labels 2022-01-14 13:46:30 +00:00
glyph added 17 commits 2022-01-14 13:46:31 +00:00
notplants commented 2022-01-14 16:11:45 +00:00
Owner
Copy Link

cool to have trimmed down the codebase so much!

I particularly like this change

Remove unnecessary context object structs (replaced with tera::Context type)
Particularly for templates which only need back, title and flash message data

reduces code and makes things more ergonomic to work with in the future
also glad to see snafu removed

here are a few comments, none blocking. could merge this PR and address these separately as inspired.

the first comment is probably the smallest and most utilitarian change.
the second I think could be a code improvement but is a larger change.
the third is just sharing my opinion, but I'm happy that you've rearranged the code into a structure that you find more appealing, since it doesn't make a big difference to me.

  1. avoid duplication of routes between build_minimal_routes and build_complete_rocket.

you can slightly restructure build_minimal_rocket so that it takes in a Rocket<Build> and returns a Rocket<Build> and thus can be called by build_complete_rocket, and then avoid duplicating routes (duplicate code in general means more typing + more chance of introducing bugs through discrepancies between the duplication).

I would also consider to change the names to mount_peachpub_routes, and mount_all_peachcloud_routes (which calls mount_peachpub_routes), and/or to have a build function which calls the correct mount function based on what environment it is, and then additionally do whatever other configuration is needed.

  1. PEACH_STANDALONE_MODE

my suggestion is a reiteration of One Config To Rule Them All, which I don't mean configs instead of env vars, what I mean is one way of handling configurations, so that for any configuration (whether peach_standalone_mode or something else) you know there is one system and one config file which handles all of them, and you dont have to arbitrarily remember which configs are handled in which ways.

if we want the ability to set env var, instead of as a a value in config.yml, I would change to using figment to handle all configurations and use a hierarchy (env > config)

and then I would have PEACH_STANDALONE_MODE as one config thats handled in the same way as all the others.

figment looks pretty easy to setup a hierarchy like this (https://docs.rs/figment/0.10.6/figment/). and then we would just need to change all the functions in config_manager to get values from figment instead of with the home-made yaml reading.

one question I've been thinking about is how this would interact with rocket's own config system (which we need to use for the rocket authentication env). I wouldn't just use rocket's env system for everything, because we also want to have access to configurations for processes running outside of a rocket context (e.g. peach-config). I have a feeling, from some reading, that there might be a way to set the file rocket reads its configuration from during the build phase, and then set this file to be /var/lib/peachcloud/config.toml , that way this config file can be read by rocket, and also read by figment from processes that are not running rocket.

but all that said, we can also keep this multiple configs now, and it could be a separate issue at some point if desired to replace config_manager with figment and then make these changes at that point.

  1. context and routes in different folders

personally my preference is still:
... have many small route files, even one file per route, so the code for each route is all together in one file along with the context, forms and data types for that route

I think having long files with many context and routes is hard to read and causes the feeling of noisiness, but small files its helpful to see everything that interacts all together, and then its super easy to make new routes, you just copy a file to a new location and edit everything in place.

but its also not my hill to die on (quoting clgbh). this also seems organized and readable to me, and now since routes and contexts are divided into multiple files based on function, instead of a single file for each, they will not get unmanageably long.

cool to have trimmed down the codebase so much! I particularly like this change > Remove unnecessary context object structs (replaced with tera::Context type) Particularly for templates which only need back, title and flash message data reduces code and makes things more ergonomic to work with in the future also glad to see snafu removed here are a few comments, none blocking. could merge this PR and address these separately as inspired. the first comment is probably the smallest and most utilitarian change. the second I think could be a code improvement but is a larger change. the third is just sharing my opinion, but I'm happy that you've rearranged the code into a structure that you find more appealing, since it doesn't make a big difference to me. _____________ 1. avoid duplication of routes between build_minimal_routes and build_complete_rocket. you can slightly restructure build_minimal_rocket so that it takes in a `Rocket<Build>` and returns a `Rocket<Build>` and thus can be called by build_complete_rocket, and then avoid duplicating routes (duplicate code in general means more typing + more chance of introducing bugs through discrepancies between the duplication). I would also consider to change the names to mount_peachpub_routes, and mount_all_peachcloud_routes (which calls mount_peachpub_routes), and/or to have a build function which calls the correct mount function based on what environment it is, and then additionally do whatever other configuration is needed. ______________ 2. PEACH_STANDALONE_MODE my suggestion is a reiteration of One Config To Rule Them All, which I don't mean configs instead of env vars, what I mean is one way of handling configurations, so that for any configuration (whether peach_standalone_mode or something else) you know there is one system and one config file which handles all of them, and you dont have to arbitrarily remember which configs are handled in which ways. if we want the ability to set env var, instead of as a a value in config.yml, I would change to using figment to handle all configurations and use a hierarchy (env > config) and then I would have PEACH_STANDALONE_MODE as one config thats handled in the same way as all the others. figment looks pretty easy to setup a hierarchy like this (https://docs.rs/figment/0.10.6/figment/). and then we would just need to change all the functions in config_manager to get values from figment instead of with the home-made yaml reading. one question I've been thinking about is how this would interact with rocket's own config system (which we need to use for the rocket authentication env). I wouldn't just use rocket's env system for everything, because we also want to have access to configurations for processes running outside of a rocket context (e.g. peach-config). I have a feeling, from some reading, that there might be a way to set the file rocket reads its configuration from during the build phase, and then set this file to be /var/lib/peachcloud/config.toml , that way this config file can be read by rocket, and also read by figment from processes that are not running rocket. but all that said, we can also keep this multiple configs now, and it could be a separate issue at some point if desired to replace config_manager with figment and then make these changes at that point. ________________ 3. context and routes in different folders personally my preference is still: ... have many small route files, even one file per route, so the code for each route is all together in one file along with the context, forms and data types for that route I think having long files with many context and routes is hard to read and causes the feeling of noisiness, but small files its helpful to see everything that interacts all together, and then its super easy to make new routes, you just copy a file to a new location and edit everything in place. but its also not my hill to die on (quoting clgbh). this also seems organized and readable to me, and now since routes and contexts are divided into multiple files based on function, instead of a single file for each, they will not get unmanageably long.
notplants commented 2022-01-14 16:27:52 +00:00
Owner
Copy Link

If looking deeper into using figment instead of the current config_manager,
some other things to consider:

  • at what point in the rocket cycle to load the configs from config (normally in rocket, I think it loads them during the build phase... but in our web app, it reads and writes configs... how do we ensure that when we've written/updated a config that the new value will be known the next URL that the user goes to... aka the config should be reloaded for every view)
  • best way to serialize and write figment configs (since we read and write them from the app)
If looking deeper into using figment instead of the current config_manager, some other things to consider: - at what point in the rocket cycle to load the configs from config (normally in rocket, I think it loads them during the build phase... but in our web app, it reads and writes configs... how do we ensure that when we've written/updated a config that the new value will be known the next URL that the user goes to... aka the config should be reloaded for every view) - best way to serialize and write figment configs (since we read and write them from the app)
notplants added 2 commits 2022-01-16 22:31:39 +00:00
notplants force-pushed web_improvements from 54359924f6 to ed6da528a2 2022-01-16 22:36:02 +00:00 Compare
notplants added 1 commit 2022-01-16 22:37:06 +00:00
notplants commented 2022-01-16 22:39:00 +00:00
Owner
Copy Link

^ I was trying to add a commit that would just remove
static/js/*
from Cargo.toml
so that cargo-deb would compile without complaining that static/js doesn't exist anymore,
but while adding this change,
it also added some changes to Cargo.lock (due to the automatic cargo fmt running as a git pre-commit hook), so I had to temporarily turn that off, then force push, so it would just send the commit with the js line removed

^ I was trying to add a commit that would just remove static/js/* from Cargo.toml so that cargo-deb would compile without complaining that static/js doesn't exist anymore, but while adding this change, it also added some changes to Cargo.lock (due to the automatic cargo fmt running as a git pre-commit hook), so I had to temporarily turn that off, then force push, so it would just send the commit with the js line removed
👍 1
notplants force-pushed web_improvements from ceed34cb49 to 4d2a3771b8 2022-01-16 22:42:15 +00:00 Compare
glyph commented 2022-01-17 09:34:43 +00:00
Author
Owner
Copy Link

@notplants

Thanks for taking a look and sharing your thoughts. I'm feeling a bit low on energy today so I'll keep my response brief:

  1. Deduplication sounds good to me. I'll do this in a separate PR as it requires some careful thought to avoid routing conflicts (for example, the Scuttlebutt status view needs to be mounted at /status for PeachPub and at /status/scuttlebutt for PeachCloud).

  2. I've copied your config-related comments over to the config issue #37. I used the simplest possible solution in this PR to avoid getting stuck in the mud. I'll respond more fully in the issue.

  3. I'm happy to revisit the separation in the future. Individual files per route still seems a bit extreme to me but our application isn't too big so maybe it would feel good once I tried it.

@notplants Thanks for taking a look and sharing your thoughts. I'm feeling a bit low on energy today so I'll keep my response brief: 1. Deduplication sounds good to me. I'll do this in a separate PR as it requires some careful thought to avoid routing conflicts (for example, the Scuttlebutt status view needs to be mounted at `/status` for PeachPub and at `/status/scuttlebutt` for PeachCloud). 2. I've copied your config-related comments over to the config issue [#37](https://git.coopcloud.tech/PeachCloud/peach-workspace/issues/37). I used the simplest possible solution in this PR to avoid getting stuck in the mud. I'll respond more fully in the issue. 3. I'm happy to revisit the separation in the future. Individual files per route still seems a bit extreme to me but our application isn't too big so maybe it would feel good once I tried it.
glyph merged commit 5d75aebf0d into main 2022-01-17 09:35:30 +00:00
glyph deleted branch web_improvements 2022-01-17 09:35:30 +00:00
notplants commented 2022-01-17 14:44:28 +00:00
Owner
Copy Link

  1. could consider mounting routes at the same URL in both cases, /status/scuttlebutt, to keep the code simpler.

  2. cool

  3. I really don't mind the way you have it setup so whatever you prefer. I think I have a general preference for functional groupings (things that interact) instead of optical groupings (things that look similar).

imo I'm also not sure the size of the app effects this pattern. If you have many routes, then you would continue to subdivide them into functional groupings in folders.

reflecting more on it, I think a key organizational preference of mine is small files. Not necessarily one route per file (but could be if they are complicated routes), but that whenever a file starts to get big, it could be a chance to come up with groupings and divide it. I'm trying to remember what the other rust reference I found was besides Plume where they had many small routes files, kind of bummed I can't find it or remember it right now

1. could consider mounting routes at the same URL in both cases, /status/scuttlebutt, to keep the code simpler. 2. cool 3. I really don't mind the way you have it setup so whatever you prefer. I think I have a general preference for functional groupings (things that interact) instead of optical groupings (things that look similar). imo I'm also not sure the size of the app effects this pattern. If you have many routes, then you would continue to subdivide them into functional groupings in folders. reflecting more on it, I think a key organizational preference of mine is small files. Not necessarily one route per file (but could be if they are complicated routes), but that whenever a file starts to get big, it could be a chance to come up with groupings and divide it. I'm trying to remember what the other rust reference I found was besides Plume where they had many small routes files, kind of bummed I can't find it or remember it right now
👍 1
notplants commented 2022-01-17 14:48:52 +00:00
Owner
Copy Link

aw right, delta was the project that had a nice implementation of one route per file https://github.com/revoltchat/delta/tree/master/src/routes

but yea seriously, less than not my hill to die, I think the current organization is also fine.

aw right, delta was the project that had a nice implementation of one route per file https://github.com/revoltchat/delta/tree/master/src/routes but yea seriously, less than not my hill to die, I think the current organization is also fine.
👍 1
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
  
bug

Something is not working

  
documentation

   
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
maintenance

Pruning and weeding

  
peach-lib

   
peach-network

   
peach-oled

   
peach-stats

   
peach-web

   
question

More information is needed

  
refactor

Code rewrite

  
wontfix

This won't be fixed

No Label
bug
documentation
duplicate
enhancement
help wanted
invalid
maintenance
peach-lib
peach-network
peach-oled
peach-stats
peach-web
question
refactor
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: PeachCloud/peach-workspace#70
No description provided.
Delete Branch "web_improvements"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?