Skip to content
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

Adds basic reachability integration in cli (Part 1/N) #1363

Closed
wants to merge 2 commits into from

Conversation

meghfossa
Copy link
Contributor

@meghfossa meghfossa commented Jan 17, 2024

Overview

This is part of stack PR. This PR will not be merged, until all stacked PR's are merged into this branch.

This PR,

  • Adds basic command line tool for reachability (reachability/extlib)
  • Adds hidden fossa reachability <PATH> command for debugging

Acceptance criteria

  • Automated build works
  • fossa reachability <PATH> can be executed (It will fail in part 1 PR)

Testing plan

git checkout master && git pull origin && git checkout feat/reachability-cli && make install-dev

after which,

fossa-dev reachability

# Note that this command will fail, as this PR does not add implementation
# so error is to be expected! You should see something like following in error message:
#
# Command error log:
#            DEBUG [reachability] Reading from file system: "/Users/megh/code/fossa-cli/"
#            DEBUG [reachability] Parsing file system: "/Users/megh/code/fossa-cli/"
#            thread 'main' panicked at extlib/reachability/src/lib.rs:31:9:
#            not yet implemented
#            note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Risks

N/A

Metrics

N/A

References

N/A

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • If this PR added docs, I added links as appropriate to the user manual's ToC in docs/README.ms and gave consideration to how discoverable or not my documentation is.
  • If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json AND I have updated example files used by fossa init command. You may also need to update these if you have added/removed new dependency type (e.g. pip) or analysis target type (e.g. poetry).
  • If I made changes to a subcommand's options, I updated docs/references/subcommands/<subcommand>.md.

pub struct Reachability {}

impl Reachability {
pub fn from_stdin() -> Result<Self> {
Copy link
Contributor Author

@meghfossa meghfossa Jan 17, 2024

Choose a reason for hiding this comment

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

I have intentionally left out implementation. This will be added in second PR tomorrow.

@meghfossa meghfossa marked this pull request as ready for review January 17, 2024 03:52
@meghfossa meghfossa requested a review from a team as a code owner January 17, 2024 03:52
Copy link
Contributor

@spatten spatten left a comment

Choose a reason for hiding this comment

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

I have one question about the proliferation about error handling libraries that we use, but I don't think that's blocking.

Other than that, looks great!

simple_logger = { version = "4.3.3", features = ["stderr", "colors"], default-features = false }
log = "0.4.17"
snippets = { git = "https://github.com/fossas/lib-snippets", version = "0.2.0", features = ["lang-java-11"] }
stable-eyre = "0.2.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried that we are using too many error handling libraries.

Broker uses error-stack. Lernie and Sherlock use this-error. Reachability-toolkit uses anyhow.

Now we're adding eyre into the mix.

If there's a good reason to use stable-eyre instead of one of the other three, then that's fine. But I wanted to double-check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the dependencies and code structure from berkelydb, but I can modify it to recent rust code one's.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Maybe we should merge this as-is, but have a quick conversation about whether we want to standardize this a bit in standup tomorrow.

I don't think it blocks this PR; we can just make the change later if we decide to

@meghfossa
Copy link
Contributor Author

Superseded by: #1372

@meghfossa meghfossa closed this Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants