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.

Sign up for GitHub

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

Jump to bottom

gRPC version service + fix falco version #872

Merged
merged 17 commits into from
Feb 7, 2020
Merged

gRPC version service + fix falco version #872

merged 17 commits into from
Feb 7, 2020

Conversation

leodido
Copy link
Member

@leodido leodido commented Oct 3, 2019 •
edited
Loading

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

NONE

What this PR does / why we need it:

Introduces a very simple unary gRPC service to obtain the Falco version.

Also this PR fixes the Falco version so that it follows the semver 2 BNF.

Which issue(s) this PR fixes:

Fixes #1039

Special notes for your reviewer:

It is intended to serve as example to help other contributors implement gRPC simple unary services.

Does this PR introduce a user-facing change?:

new: gRPC version API
fix: version follows semver 2 BNF

@poiana poiana requested review from krisnova and mstemm October 3, 2019 16:52
@poiana poiana added the size/L label Oct 3, 2019
@leodido leodido requested a review from fntlnz October 4, 2019 09:43
@stale
Copy link

stale bot commented Dec 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 29, 2019
@stale stale bot closed this Jan 5, 2020
@fntlnz
Copy link
Contributor

fntlnz commented Jan 20, 2020

/reopen

@poiana poiana reopened this Jan 20, 2020
@poiana
Copy link

poiana commented Jan 20, 2020

@fntlnz: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@stale stale bot removed the wontfix label Jan 20, 2020
@fntlnz fntlnz changed the base branch from dev to master February 3, 2020 15:14
@leodido
docs(tests/falco): license for webserver unit tests
49e8a2e 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
build: extract version pieces
922e1cd 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
update(userspace/falco): define version part variables
1494ff5 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
new(userspace/falco): protobuf for gRPC version service
b497530 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
docs(userspace/falco): document version protobuf
0359cbe 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
build: compile version proto
d481280 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido leodido mentioned this pull request Feb 5, 2020
Merged
krisnova
krisnova previously approved these changes Feb 5, 2020
View reviewed changes
Copy link
Contributor

@krisnova krisnova left a comment

Choose a reason for hiding this comment

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

Few comments - otherwise looks good

userspace/falco/grpc_request_context.cpp Outdated Show resolved Hide resolved
userspace/falco/grpc_request_context.h

void start(server* srv);
void process(server* srv);
void end(server* srv, bool isError);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about signal handling?

@poiana poiana added the lgtm label Feb 5, 2020
@poiana
Copy link

poiana commented Feb 5, 2020

LGTM label has been added.

Git tree hash: 937a9ae938eac5d2f10e38f2c9f0486694fcadf9

@poiana poiana added the approved label Feb 5, 2020
@leodido
Copy link
Member Author

leodido commented Feb 5, 2020 •
edited
Loading

Refs #818

fntlnz
fntlnz reviewed Feb 6, 2020
View reviewed changes
userspace/falco/grpc_server.cpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@leodido
fix(userspace): ensure threadiness is gt 0
2337183 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
update(cmake/modules): module to detect Falco version from the git index
8bf26e5 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
update(userspace/falco): major, minor, patch are digits, so use integers
3b504fe 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
docs(userspace/falco): document gRPC errors and actions
b3e8b2c 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
update(userspace/falco): use mutable proto fields where applicable
a738107 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido
chore: stick with the error messages we have
d7549ab 
Because we can't easily change the integration test fixtures.

Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@poiana poiana added the lgtm label Feb 7, 2020
@poiana
Copy link

poiana commented Feb 7, 2020

LGTM label has been added.

Git tree hash: 90f38065a4692bc3b9dfda7641d0c2156bb69332

@poiana poiana added the approved label Feb 7, 2020
@krisnova
Copy link
Contributor

krisnova commented Feb 7, 2020

/lgtm

@poiana
Copy link

poiana commented Feb 7, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, kris-nova

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@leodido leodido changed the title gRPC version service gRPC version service + fix falco version Feb 7, 2020
@poiana poiana merged commit 253ff64 into master Feb 7, 2020
@poiana poiana deleted the feature/grpc-version-svc branch February 7, 2020 10:29
@fntlnz fntlnz added this to the 0.19.1 milestone Feb 17, 2020
@fntlnz fntlnz mentioned this pull request Feb 23, 2020
Closed
@fntlnz
Copy link
Contributor

fntlnz commented Feb 23, 2020 •
edited
Loading

@leodido i have a question on this: can the version service be enabled/disabled via config like output does ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fntlnz fntlnz fntlnz approved these changes

@krisnova krisnova krisnova left review comments

@mstemm mstemm Awaiting requested review from mstemm

Assignees

@fntlnz fntlnz

@krisnova krisnova

Labels
approved dco-signoff: yes kind/feature lgtm release-note size/XXL
Projects
None yet
Milestone
0.20.0
Development

Successfully merging this pull request may close these issues.

Fix the Falco version
4 participants
@leodido @fntlnz @poiana @krisnova