Handler tests #11
Handler tests #11
Conversation
albertpastrana
force-pushed
the
handler-tests
branch
2 times, most recently
from
bc9af1e
to
bdf0e39
Compare
git/src/lib.rs
Outdated
| } | ||
|
|
||
| fn git_cat_file_err(repo_path: &Path, reference: &str, filename: &str) -> git2::Error { | ||
| let gh = GitHelper {}; |
There was a problem hiding this comment.
Replace with git_cat_file helper?
There was a problem hiding this comment.
sure, I forgot 🤦♂️
| @@ -0,0 +1,12 @@ | |||
| [package] | |||
There was a problem hiding this comment.
I dunno whether handlers should be it's own project or if it should just be a module within the server project — feels a little too split up now, and I suspect the code-smell is the fact they share all the dependencies.
There was a problem hiding this comment.
yup, agree, the main reason for this was to be able to add tests in the documentation, happy to move it back to server
There was a problem hiding this comment.
We discussed to leave this till later because we want to see what the documentation looks like with error handling first. The reason why this has been split out is because we can't do doctests in the server project which is building a bin — so if we need doctests because of the error handling, it may make plenty of sense to leave them split-up. 👍
handlers/Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| git = { path = "../git" } | ||
| actix-web = "0.7.*" |
There was a problem hiding this comment.
As in the other PR, update these version ranges to be X.Y.Z or ^X.Y.Z ranges, rather than use * or X.Y or just X.
| fn handle(&self, req: &HttpRequest<S>) -> Self::Result { | ||
| let path_params = Path::<PathParams>::extract(req).unwrap(); | ||
| let query_params = Query::<QueryParams>::extract(req).unwrap(); | ||
| let repo_path: PathBuf = [&self.repo_root, &path_params.repo].iter().collect(); |
There was a problem hiding this comment.
I think this is wildly unsafe — there's nothing stopping me from pointing it at /etc/passwd or some actual repo somewhere else on your system and play about until I get an error message or response to contain something interesting.
The way to fix this is to have a config that goes name -> path and require the param to mention the name, looking up the path server side. That is, effectively, whitelist the repos this tool can point to.
Perhaps the toml crate (which provides deserialisation of a toml file using serde) works? https://crates.io/crates/toml
There was a problem hiding this comment.
I do like the idea. For the sake of moving forward (and given that this is not yet production ready code) I propose to create an issue to have a list of allowed repos. This could be automatically populated from the repo_root path. And we could also expose an endpoint that returns the list of repos configured.
| res: String, | ||
| } | ||
|
|
||
| impl GitOps for TestGitOps { |
There was a problem hiding this comment.
Ah nice one, this is actually quite nice! I love trait + impl in Rust, it's so powerful.
albertpastrana
force-pushed
the
handler-tests
branch
2 times, most recently
from
89801a9
to
c921aa5
Compare
Signed-off-by: Albert Pastrana <albert.pastrana@intenthq.com>
Signed-off-by: Albert Pastrana <albert.pastrana@intenthq.com>
Signed-off-by: Albert Pastrana <albert.pastrana@intenthq.com>
albertpastrana
force-pushed
the
handler-tests
branch
from
32fba70
to
c8b5c25
Compare
| @@ -0,0 +1,12 @@ | |||
| [package] | |||
There was a problem hiding this comment.
We discussed to leave this till later because we want to see what the documentation looks like with error handling first. The reason why this has been split out is because we can't do doctests in the server project which is building a bin — so if we need doctests because of the error handling, it may make plenty of sense to leave them split-up. 👍
git/src/lib.rs
Outdated
|
|
||
| repo.find_blob(te.id()).map(|x| x.content().to_owned()) | ||
| pub struct GitHelper; |
There was a problem hiding this comment.
I wonder whether this should have another name or not seeing as it's public. Maybe like LibGitOps, which hints as to how it's doing it's work (which is the dual of the trait name GitOps, which hints that the ops need to do something Git related however they want)?
Signed-off-by: Albert Pastrana <albert.pastrana@intenthq.com>
This is work on top of #3 and should be reviewed after #9, as it's work done from that branch.