Skip to content
This repository has been archived by the owner on Sep 21, 2023. It is now read-only.

Create initial Skeleton of the shipper and gRPC service #18

Merged
merged 8 commits into from
Mar 31, 2022

Conversation

fearful-symmetry
Copy link
Contributor

This is part of #6.

This creates the initial skeleton of the shipper, with a basic config and CLI manager, and the a super basic gRPC server based on #5. I'm not sure how long some of this code is going to "live", but the idea was to have all the basic building-blocks in place, so we can start writing around it. Right now, you can compile it, run it, and talk to the gRPC service, which won't really do anything besides print some log messages.

This is my first time actually working directly with gRPC, so if something in the actual gRPC components seems...weird, that's why. Going off of @cmacknz 's suggestion, I vendored the imported protobuf types, but I'm sure there's a more idiomatic way to do the vendoring and build.

The included magefile is also very basic, as a considerable deal of the mage glue code, and some of the targets used by agent aren't in a importable library, but live in the main magefile.go. I'm not a big fan of copy+pasting huge chunks of code, even if it's for something like this. So we'll need some refactoring of the mage/dev-tools code in https://github.com/elastic/elastic-agent before we can "Standardize" this magefile more.

@fearful-symmetry fearful-symmetry added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Mar 30, 2022
@fearful-symmetry fearful-symmetry requested review from cmacknz, kvch, faec and a team March 30, 2022 18:01
@fearful-symmetry fearful-symmetry self-assigned this Mar 30, 2022
@cmacknz cmacknz requested a review from rdner March 30, 2022 18:05
magefile.go Show resolved Hide resolved
shipper/shipper.proto Outdated Show resolved Hide resolved
@@ -0,0 +1,153 @@
# options for analysis running
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a template in the other repositories to allow substituting in at least the go-version from the .go-version file we haven't added to this repo yet.

We should probably do the same. Here's the commit that added linting to beats for example: elastic/beats@0a749b5

To get CI to run it you'd just need to drop the github action in: https://github.com/elastic/elastic-agent-libs/blob/main/.github/workflows/golangci-lint.yml

We don't need to do the linter setup as part of this commit if it is adding too much work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's interesting. I coped the out out of the elastic-agent repo, which didn't have the autogen warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, copied in the CI files and fixed the file generation, not sure if there's anything else we need to do to make the linter happy.

Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to exclude generated go files (*.pb.go) from linting (see skip-files in the docs). The change must be made in the template and then rendered via mage linter:updategoversion.

And we definitely need to try run the linter after the final config is rendered just to make sure the next PR would not fail (the current PR is not running the linter workflow yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, added the skip-files section.

And we definitely need to try run the linter after the final config is rendered

You mean the protobuf files? Right now the mage command that generates that isn't in CI, I assume we want it to be?

Copy link
Member

Choose a reason for hiding this comment

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

  1. Yes we will want the command to generate the protobuf files to run in CI, otherwise the CI build will fail when we turn it on.
  2. @rdner is suggesting that we enable the linter as part of these changes. You can just create the github action to do this by copying one from our other repositories: https://github.com/elastic/elastic-agent-libs/blob/main/.github/workflows/golangci-lint.yml.

Copy link
Member

@rdner rdner Mar 31, 2022

Choose a reason for hiding this comment

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

@cmacknz the GitHub action is already a part of this PR. I just thought would be nice to adjust the configuration and fix issues in the current code (if any). For that you need to run the linter locally because when you add the Github Action (in this PR) it's not activated until merged into main. Otherwise all these linter issues would surface in a next PR.

@jlind23 jlind23 requested a review from a team March 31, 2022 06:37
magefile.go Show resolved Hide resolved
@fearful-symmetry
Copy link
Contributor Author

Alright, merging this now, since I'd rather not feel like I'm blocking other people that have work to do. Still deciding how to "borrow" a lot of the related code currently in elastic-agent.

@fearful-symmetry fearful-symmetry merged commit a0f5c1b into elastic:main Mar 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants