Add login page (Backend) #16

archeoss · 2023-10-05T06:36:42Z

partly resolves the #2 issue

archeoss · 2023-10-05T06:36:58Z

Add login page (Backend) #16 : 2 dependent PRs (#17 , #18 ) 👈
11 configure cicd #13
5 create dockerfile and docker compose to simplify deployment #12
Create initial project structure (Frontend) #10
Create initial project structure (Backend) #9 : 1 other dependent PR (#15 )
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @archeoss and the rest of your teammates on Graphite

archeoss · 2023-10-12T20:18:01Z

Need some time to tidy up everything, but generally everything should be done for this PR

backend/src/connector/api.rs

backend/src/connector/client.rs

backend/src/connector/mod.rs

backend/src/main.rs

ikopylov · 2023-12-05T21:58:16Z

backend/src/services/auth.rs

@@ -0,0 +1,349 @@
+use super::prelude::*;
+
+pub type HttpClient = ContextWrapper<


Can you explain what's going on with ClientContext here? Why do you declare the Client without context (DropContextService) and then wrap it with context? What is the reason that Client was not declared directly with the context?

In theory, we can wrap any context with any context provider (for example, the one that reads\writes context from\to file, and not that just wraps the context and keeps it in-memory). Practically, It could be solved with generic types and some methods, not the whole wrapper, but since that was the part of auto-generated code, I decided to keep it this way
Also we can replace the context on specific calls, not changing the wrapped one, but I fail to see the situation where that's actually useful (and the wrapper part too)

I understand that generics are used for better flexibility. But here a specific type with all substitutions is declared. Why are additional layers needed here? Why can't it be written like this:

pub type HttpClient = ContextWrapper< Client< hyper::Client<HttpConnector, Body>, ClientContext, Basic, >, ClientContext, >;

backend/src/services/auth.rs

backend/src/models/shared.rs

backend/src/services/auth.rs

ikopylov · 2024-01-25T01:16:57Z

CHANGELOG.md

@@ -5,8 +5,10 @@ Bob Management GUI changelog
 ## [Unreleased]

 #### Added
+


Unnecessary empty line

ikopylov · 2024-01-25T01:50:09Z

backend/src/connector/mod.rs

+}
+
+#[derive(Clone)]
+pub struct BobClient<Client: ApiNoContext<ClientContext> + Send + Sync> {


Perhaps, it is better to call it BobClusterClient, because inside it contains clients for all nodes

ikopylov · 2024-01-25T02:05:16Z

backend/src/router.rs

@@ -136,22 +181,38 @@ impl<V, D, S, B> Deref for ContextRouter<V, D, S, B> {
 }
 }

+#[cfg(all(feature = "swagger", debug_assertions))]
+pub trait RouterApiExt<S = (), B = Body, E = Infallible> {


I see no difference between this trait and the one below. Looks like the separation based on cfg is not needed on trait declaration level

ikopylov · 2024-01-25T02:14:05Z

backend/src/router.rs

+ }
+}
+
+#[cfg(all(feature = "swagger", debug_assertions))]


The separation of implementations occurs at different levels. Here the struct level is used, and above - the function level. It is better to unify places, where cfg is applied. It seems that it is better to make a separation at the function level here

The only difference seems to be the call to check_api, I suggest to cfg only the call and the function itself

ikopylov · 2024-01-25T02:14:47Z

backend/src/router.rs

 }

+#[cfg(not(all(feature = "swagger", debug_assertions)))]


Implementations are also look identical

ikopylov · 2024-01-25T16:26:15Z

backend/src/services/auth.rs

+ Ok(res)
+}
+
+#[async_trait]


Looks like async_trait can be removed here

ikopylov · 2024-01-25T16:44:56Z

backend/src/services/auth.rs

+ }
+}
+
+#[async_trait]


Looks like async_trait is not needed here

idruzhitskiy · 2024-01-29T11:41:30Z

backend/src/connector/client.rs

+ let connector = Connector::builder();
+
+ let client_service = match scheme.as_str() {
+ "http" => HyperClient::Http(hyper::client::Client::builder().build(connector.build())),


What about https, can it be added? bob technically supports it

idruzhitskiy · 2024-01-29T11:55:19Z

backend/src/connector/mod.rs

+
+ let cluster: HashMap<NodeName, Arc<_>> = nodes
+ .iter()
+ .filter_map(|node| HttpClient::from_node(node, &bob_data.hostname, context.clone()))


API ports can be different on each node, and they are not known to other nodes. There should be a way to override default ports. In bob-tools it's done with argument api-port, Override default api port for the node. E.g. node1:80,node2:8000. Wildcard char (*) can be used to set port for all nodes.

idruzhitskiy · 2024-01-29T11:55:50Z

backend/src/connector/mod.rs

+ }
+ }
+
+ /// Probes connection to the Bob's main connected node


Comment out of sync?

idruzhitskiy · 2024-01-29T12:14:33Z

backend/src/connector/mod.rs

+
+ // NOTE: Can (and should) the API interface mutate?..
+ /// Connection,
+ main: Arc<Client>,


What is the reason to have single main node? Why not any other node in cluster?

archeoss force-pushed the 11-configure-cicd branch from 4c06da4 to e5801a4 Compare October 5, 2023 10:40

archeoss force-pushed the 2-login-page branch from 6f7a174 to a1ef4f8 Compare October 5, 2023 10:40

archeoss force-pushed the 11-configure-cicd branch from 27e4ea7 to 50b9e0b Compare October 5, 2023 10:57

archeoss force-pushed the 2-login-page branch from 220602b to dc71a9f Compare October 5, 2023 10:57

archeoss force-pushed the 11-configure-cicd branch 6 times, most recently from 639e9a0 to 45c472b Compare October 9, 2023 16:18

archeoss force-pushed the 2-login-page branch from 8b1ce1e to 1ec7937 Compare October 12, 2023 20:16

archeoss marked this pull request as ready for review October 12, 2023 20:17

archeoss requested a review from ikopylov October 12, 2023 20:18

archeoss closed this Oct 12, 2023

archeoss reopened this Oct 12, 2023

archeoss force-pushed the 11-configure-cicd branch from 66c6be0 to 1ec6c17 Compare October 12, 2023 23:14

archeoss force-pushed the 2-login-page branch 2 times, most recently from 22b2cbb to 3c6aff8 Compare October 13, 2023 14:44

archeoss changed the title ~~Add login page~~ Add login page (Backend) Oct 13, 2023

archeoss linked an issue Oct 13, 2023 that may be closed by this pull request

Login page #2

Open

archeoss mentioned this pull request Oct 13, 2023

Add login page (Frontend) #17

Open

ikopylov reviewed Oct 17, 2023

View reviewed changes

backend/src/connector/api.rs Outdated Show resolved Hide resolved

ikopylov reviewed Oct 17, 2023

View reviewed changes

backend/src/connector/client.rs Show resolved Hide resolved