-
Notifications
You must be signed in to change notification settings - Fork 2
Add license scan report and status #16
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
base: master
Are you sure you want to change the base?
Conversation
Signed off by: fossabot <badges@fossa.com>
|
Welcome @fossabot! It looks like this is your first PR to kmesh-net/waypoint 🎉 |
Summary of ChangesHello @fossabot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces FOSSA license scanning integration for better compliance tracking and updates the project's Envoy dependency to a specific fork. Additionally, it enhances the existing Zipkin tracing within the Mixer HTTP filter to provide more detailed service context in the generated traces. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request, apparently created by the FOSSA tool, introduces a license scan badge and report. However, it also includes several other significant changes that are not described. The Envoy dependency is switched to a fork, and new tracing-related code is added to the mixer HTTP filter. This new tracing code appears to be incomplete and likely contains bugs. Additionally, there are some minor formatting issues in the README.
| void updateSpan() const { | ||
| Tracing::Span& activeSpan = decoder_callbacks_->activeSpan(); | ||
| if (Tracing::ZipkinSpan *zipkinSpan = dynamic_cast<Tracing::ZipkinSpan*>(&activeSpan)) { | ||
| Zipkin::Span& span = zipkinSpan->span(); | ||
| span.setSourceService(); //need source service name. | ||
| span.setDestinationService(); //need destination service name. | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of updateSpan appears to be a placeholder and is incorrect.
- Incorrect calls: The calls to
span.setSourceService()andspan.setDestinationService()are made without arguments, while the inline comments (//need source service name.) indicate they are required. This will likely cause a build failure or a runtime bug, breaking tracing functionality. The function's purpose is to use data from Mixer's response, but the implementation does not extract or use any such data. - Tight Coupling: The use of
dynamic_casttoTracing::ZipkinSpantightly couples this code to a specific tracing implementation. Consider extending theTracing::Spaninterface to make this more generic and maintainable.
This is a critical issue that needs to be addressed before merging.
| remote = "https://github.com/amalgam8/envoy.git", | ||
| commit = "c89a99e893b633e97419d306e0072f07bd2200fa", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change switches the envoy dependency from the official lyft/envoy repository to the amalgam8/envoy fork. This is a major change that should be clearly explained in the pull request description. Using a fork can introduce risks related to maintenance, security, and falling behind the upstream project. Please add a comment to the git_repository rule or update the PR description to justify why this fork is necessary.
| contribute to any of the Istio components, including the proxy, please | ||
| see the Istio [contribution guidelines](https://github.com/istio/istio/blob/master/CONTRIBUTING.md). | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
|
|
||
| ## License | ||
| [](https://app.fossa.io/projects/git%2Bgithub.com%2Frshriram%2Fproxy?ref=badge_large) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your FOSSA integration was successful! Attached in this PR is a badge and license report to track scan status in your README.
Below are docs for integrating FOSSA license checks into your CI: