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 exposure of locks externally from the grpc server struct #101

Merged
merged 1 commit into from
May 9, 2022

Conversation

chrisdoherty4
Copy link
Member

Fixes #93 that we originally thought could segfault. The json package actually returns an error about an unsupported type.

Nonetheless, the issue with lock exposure and management external to an owning struct is dangerous and has been fixed. The endpoint that passed an incorrect type has removed given no-body could've been using it usefully.

@chrisdoherty4 chrisdoherty4 force-pushed the feature/refactor-grpc branch from ebb0029 to c59fd0a Compare May 9, 2022 00:59
@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #101 (f99b1f2) into main (6c5bc04) will decrease coverage by 0.02%.
The diff coverage is 17.24%.

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
- Coverage   42.56%   42.53%   -0.03%     
==========================================
  Files           8        8              
  Lines         592      583       -9     
==========================================
- Hits          252      248       -4     
+ Misses        309      304       -5     
  Partials       31       31              
Impacted Files Coverage Δ
http/handlers.go 47.67% <0.00%> (+2.37%) ⬆️
grpc/server.go 54.43% <45.45%> (-2.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c5bc04...f99b1f2. Read the comment docs.

@chrisdoherty4 chrisdoherty4 changed the title Fix exposure of locks externally to the grpc server struct Fix exposure of locks externally from the grpc server struct May 9, 2022
@chrisdoherty4 chrisdoherty4 force-pushed the feature/refactor-grpc branch from c59fd0a to acda117 Compare May 9, 2022 11:26
micahhausler
micahhausler previously approved these changes May 9, 2022
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

Changes LGTM

jacobweinstock
jacobweinstock previously approved these changes May 9, 2022
@jacobweinstock jacobweinstock force-pushed the feature/refactor-grpc branch from acda117 to f88fb3f Compare May 9, 2022 15:39
@jacobweinstock jacobweinstock added the ready-to-merge Signal to Mergify to merge the PR. label May 9, 2022
@jacobweinstock jacobweinstock removed request for nshalman and mmlb May 9, 2022 15:40
@chrisdoherty4 chrisdoherty4 dismissed stale reviews from jacobweinstock and micahhausler via 9e30b58 May 9, 2022 16:08
@chrisdoherty4 chrisdoherty4 force-pushed the feature/refactor-grpc branch from f88fb3f to 9e30b58 Compare May 9, 2022 16:08
@chrisdoherty4 chrisdoherty4 force-pushed the feature/refactor-grpc branch from 9e30b58 to f99b1f2 Compare May 9, 2022 18:17
@mergify mergify bot merged commit 17530ef into tinkerbell:main May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling of subscription query could segfault
3 participants