Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: added E2E test and docs for ConsumerModificationProposal #1949
feat!: added E2E test and docs for ConsumerModificationProposal #1949
Changes from 4 commits
4828649
4a7b417
59589ba
dc4b913
0cacc45
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Add a comma after 'Otherwise' for grammatical correctness.
Committable suggestion
Tools
LanguageTool
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.
Expand the
ConsumerModificationProposal
section to include transitioning between Top N and Opt-In chains.Consider adding blank lines around headings for better markdown formatting.
Committable suggestion
Tools
LanguageTool
Markdownlint
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.
Add FAQ entries about modifying power shaping parameters and transitioning between Top N and Opt-In chains using
ConsumerModificationProposal
.Consider adding blank lines around headings for better markdown formatting.
Committable suggestion
Tools
Markdownlint
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.
Because this PR was touching docs as well, fixed those stale links.
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.
Update URLs to point to the correct resources.
Consider adding a comma after "chains" for better readability.
Committable suggestion
Tools
LanguageTool
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.
Because this PR was touching docs as well, fixed those stale links.
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.
Address potential security vulnerability with unsafe quoting.
The use of
fmt.Sprintf
to dynamically generate a bash command with user-controlled input can lead to command injection vulnerabilities. Consider using more secure alternatives or ensuring that inputs are properly sanitized.Tools
GitHub Check: CodeQL
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.
Review the implementation of
SubmitConsumerModificationProposalAction
.The function
submitConsumerModificationProposal
handles the submission of a consumer modification proposal. However, there are several issues and improvements to consider:log.Fatal
for error handling is not recommended in production code as it will stop the execution of the program. Consider returning errors to the caller instead.fmt.Sprintf
with user-controlled input can lead to command injection vulnerabilities. Ensure that inputs are properly sanitized before use.Tools
golangci-lint
GitHub Check: CodeQL
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.
Ensure comprehensive test coverage for
partial-set-security-modification-proposal
.The new test case should thoroughly test all aspects of the security modifications it aims to cover. Consider adding more detailed steps or checks to ensure comprehensive coverage.
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.
Address potential non-determinism in map iteration.
The iteration over
tr.chainConfigs
map in the switch case forConsumerModificationProposal
could lead to non-deterministic behavior as the order of map iteration in Go is not guaranteed. Consider sorting the keys or using a slice if the order is important for the application logic.Committable suggestion
Tools
GitHub Check: CodeQL