Skip to content

vickyctl #27

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

Merged
merged 39 commits into from
May 15, 2024
Merged

vickyctl #27

merged 39 commits into from
May 15, 2024

Conversation

Kek5chen
Copy link
Contributor

@Kek5chen Kek5chen commented May 2, 2024

Implements #9

@Kek5chen Kek5chen added the enhancement New feature or request label May 2, 2024
@Kek5chen Kek5chen added this to the v0.1 milestone May 2, 2024
@Kek5chen Kek5chen self-assigned this May 2, 2024
@Kek5chen Kek5chen force-pushed the feature/vickyctl branch from f0f5e7b to 4199c9a Compare May 10, 2024 07:49
@Kek5chen
Copy link
Contributor Author

@johannwagner no idea why nix pipeline fails

@johannwagner
Copy link
Contributor

I will fix it later.

@johannwagner
Copy link
Contributor

Move it from Draft, if you are ready 🙌

@Kek5chen Kek5chen marked this pull request as ready for review May 14, 2024 11:58
@johannwagner
Copy link
Contributor

cargo add adds the newest available version into Cargo.toml, which might be too new for our nixpkgs Version.

@johannwagner johannwagner self-requested a review May 15, 2024 06:12
use yansi::Paint;

#[derive(Debug)]
pub enum Error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You introduced your own error type here and derived manually from it.
In vicky, we use https://docs.rs/thiserror/latest/thiserror/.

Is there a reason for an own error implementation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is. thiserror doesn't give me enough granularity over the error messages the way I would like there to be.

@johannwagner
Copy link
Contributor

Solid Work!

After integration in our pipelines and repositories, we might want to tweak the loading of AppContext a bit, but this may not be part of this pull request.

@johannwagner johannwagner merged commit 003d043 into main May 15, 2024
4 checks passed
@Kek5chen Kek5chen deleted the feature/vickyctl branch June 4, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants