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

Want to help? Just try out `cargo crev` and give feedback. #37

Open
dpc opened this Issue Dec 3, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@dpc
Owner

dpc commented Dec 3, 2018

cargo-crev is kind of working already. In a sense it's even quite feature complete (alpha quality though)

See https://github.com/dpc/crev/tree/master/cargo-crev for instructions.

@dpc dpc changed the title Want to help? Just try `cargo crev` Want to help? Just try out `cargo crev` and give feedback. Dec 3, 2018

@tylerlaberge

This comment has been minimized.

tylerlaberge commented Dec 4, 2018

Just went through building this project and trying this out and here are some things I ran into (for the most part it seems to be working and looks great)

Build notes:

Had some issues building this because of some dependencies I didn't have installed, specifically I did not have OpenSSL installed or a C compiler needed for argonautica. These were resolved with

sudo apt-get update
sudo apt-get upgrage

# openssl deps
sudo apt-get install openssl libssl-dev

# argonautica deps
sudo apt-get install clang llvm-dev libclang-dev

After getting those dependencies installed the project compiled successfully. Perhaps these dependencies should be called out in the build instructions?

cargo-crev notes:

  1. cargo crev verify should color code trusted vs not trusted dependencies, currently its a little hard to parse since its all just one color. Green for trusted, Red for not trusted would be cool (maybe yellow for low/medium trusted?)

  2. It's a little hard to see what has been reviewed with cargo crev review but not commited/pushed yet. You can do a cargo crev db git status to see files that haven't been pushed, but the filenames don't give any indication of the package the review was for. It would be cool if you could do something like cargo crev status and get a list of dependency names that you have reviewed but not pushed yet.

  3. cargo crev db git push does not work for me.

$ cargo crev db git push
fatal: The current branch master has no upstream branch.
To push the current branch and set the remote as upstream, use

git push --set-upstream origin master

if I try the set-upstream command I get permission denied.

  1. Finally, I wonder if the git commands should be kind of abstracted in some way?

So instead of

cargo crev db git add/cargo crev db git commit/cargo crev db git push

it could instead just be something like

cargo crev db save (adds/commits)
cargo crev db publish (pushes to remote)

Reason being is I don't exactly see the use case of having all the git commands available, seems like users really would only need the ability to save and publish to me.

Overall, really cool project and I'm excited to see where this goes (and the code looks well written too which is great), hope this feedback helps, when I have time I will try to make some contributions :)

@dpc

This comment has been minimized.

Owner

dpc commented Dec 4, 2018

I've just improved cargo crev verify output . Also created #39 for colors and other improvements.

It's a little hard to see what has been reviewed

cargo crev db git diff HEAD should show you this. Basically all git commands just work, so it should be fairly flexible. If this is not enough, we can definitely add something. It's just the matter of figuring out a consistent usage model.

cargo crev db git push does not work for me.

#36 - let me know in case it doesn't help (open a new issue with the actual error)

Finally, I wonder if the git commands should be kind of abstracted in some way?

I'm not sure myself. I am proficient git user, so for me issuing raw git commands is easier, I have my own shortcuts, additional commands etc.

But I guess not every user has to feel this way, so some helpers for most common workflows, could be useful.

Reason being is I don't exactly see the use case of having all the git commands available

For me, it enables any more powerful workflows and tools. Pushing to multiple places, changing branches, etc. Also - it was quicker to implement a generic git wrapper, than figuring out all the details. :D

@dpc

This comment has been minimized.

Owner

dpc commented Dec 4, 2018

@tylerlaberge I was rushing to go to sleep yesterday, but I really appreciate the feedback. Thank you!

@kornelski

This comment has been minimized.

kornelski commented Dec 18, 2018

In the screencast the "and now you review code" part is handwaved. To me, this is the hard part. I know I can't trust package's source code as shown on github, and need to review the actual crate file from crates.io, but getting it is cumbersome. Then I have to ensure I reviewed every file and haven't overlooked anything, and that the crate I dug up is actually the same crate that I'm running trust commands for.

Could you have an interactive mode, similar to git add -p, where you display each file in the terminal, and ask "Is this good? (y/n)" and automatically create review data based on that?

@dpc

This comment has been minimized.

Owner

dpc commented Dec 18, 2018

@kornelski Yes. Of course the review part is the time consuming and difficult one. Part of the reason the thoroughness field exists. You can just glance through files, and not even review all, and just mark the thoroughness as none or low. It's still far better than if noone was reviewing them at all. Eventually, when there's enough reviewers and reviews floating around, there will be a time to start getting more methodological about it. I do have some plans for per-file reviews (in form of Code Review Proofs) but it's not complete atm. We could also add some helpful tooling to at least help mark files which were reviewed for the convenience of the reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment