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

fix: Fix panic on success of Add/RemoveP2PCollections #1297

Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1296

Description

Fixes a panic on success of Add/RemoveP2PCollections. The auto generated code expects a non nil result if error is nil, and will panic otherwise.

The auto generated code expects a non nil result if error is nil, and will panic otherwise.
The auto generated code expects a non nil result if error is nil, and will panic otherwise.
@AndrewSisley AndrewSisley added bug Something isn't working area/p2p Related to the p2p networking system action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary labels Apr 5, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.5 milestone Apr 5, 2023
@AndrewSisley AndrewSisley requested a review from a team April 5, 2023 19:32
@AndrewSisley AndrewSisley self-assigned this Apr 5, 2023
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #1297 (6267a1c) into develop (c546ca4) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1297      +/-   ##
===========================================
- Coverage    70.51%   70.45%   -0.06%     
===========================================
  Files          184      184              
  Lines        17817    17825       +8     
===========================================
- Hits         12563    12558       -5     
- Misses        4300     4314      +14     
+ Partials       954      953       -1     
Impacted Files Coverage Δ
net/api/service.go 2.50% <0.00%> (-0.28%) ⬇️

... and 7 files with indirect coverage changes

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

I was just about to push this change. Thanks!

@AndrewSisley AndrewSisley merged commit e1d7545 into develop Apr 5, 2023
@AndrewSisley AndrewSisley deleted the sisley/fix/I1296-p2p-service-add-return-success branch April 5, 2023 19:43
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
* Return empty value on AddP2PCollections success

The auto generated code expects a non nil result if error is nil, and will panic otherwise.

* Return empty value on RemoveP2PCollections success

The auto generated code expects a non nil result if error is nil, and will panic otherwise.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…1297)

* Return empty value on AddP2PCollections success

The auto generated code expects a non nil result if error is nil, and will panic otherwise.

* Return empty value on RemoveP2PCollections success

The auto generated code expects a non nil result if error is nil, and will panic otherwise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary area/p2p Related to the p2p networking system bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

defradb client rpc p2pcollection add panics given valid, existing, schema id
2 participants