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

Add Go code generated for api_v2 #116

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions .github/workflows/ci-lint-test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
name: Lint Checks

on:
push:
branches: [main]

pull_request:
branches: [main]

concurrency:
group: ${{ github.workflow }}-${{ (github.event.pull_request && github.event.pull_request.number) || github.ref || github.run_id }}
cancel-in-progress: true

# See https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions
permissions: # added using https://github.com/step-security/secure-workflows
contents: read

jobs:
generated-files-check:
runs-on: ubuntu-latest
steps:
- uses: step-security/harden-runner@91182cccc01eb5e619899d80e4e971d6181294a7 # v2.10.1
with:
egress-policy: audit # TODO: change to 'egress-policy: block' after a couple of runs

- uses: actions/checkout@d632683dd7b4114ad314bca15554477dd762a938 # v4.2.0
with:
submodules: recursive

- uses: actions/setup-go@3041bf56c941b39c61721a86cd11f3bb1338122a # v5.2.0
with:
go-version: 1.23.x

- name: Verify Protobuf types are up to date
run: make new-proto && git diff --name-status --exit-code
15 changes: 15 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,21 @@ PROTOC_INTERNAL := $(PROTOC) \
--csharp_out=internal_access,base_namespace:${PROTO_GEN_CSHARP_DIR} \
--python_out=${PROTO_GEN_PYTHON_DIR}

GO=go
GOOS ?= $(shell $(GO) env GOOS)
GOARCH ?= $(shell $(GO) env GOARCH)

# sed on Mac does not support the same syntax for in-place updates as sed on Linux
# When running on MacOS it's best to install gsed and run Makefile with SED=gsed
ifeq ($(GOOS),darwin)
SED=gsed
else
SED=sed
endif

# import other Makefiles after the variables are defined
include Makefile.Protobuf.mk

.PHONY: proto
proto: proto-prepare proto-api-v2 proto-api-v3

Expand Down
70 changes: 70 additions & 0 deletions Makefile.Protobuf.mk
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# Copyright (c) 2023 The Jaeger Authors.
# SPDX-License-Identifier: Apache-2.0

# Generate gogo, swagger, go-validators, gRPC-storage-plugin output.
#
# -I declares import folders, in order of importance. This is how proto resolves the protofile imports.
# It will check for the protofile relative to each of thesefolders and use the first one it finds.
#
# --gogo_out generates GoGo Protobuf output with gRPC plugin enabled.
# --govalidators_out generates Go validation files for our messages types, if specified.
#
# The lines starting with Mgoogle/... are proto import replacements,
# which cause the generated file to import the specified packages
# instead of the go_package's declared by the imported protof files.
#

DOCKER_PROTOBUF_VERSION=0.5.0
DOCKER_PROTOBUF=jaegertracing/protobuf:$(DOCKER_PROTOBUF_VERSION)
Comment on lines +17 to +18
Copy link
Member

Choose a reason for hiding this comment

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

there are already vars defined in the main Makefile for the same concept, should be using them.

# PROTOC := ${DOCKER} run --rm -u ${shell id -u} -v${PWD}:${PWD} -w${PWD} ${DOCKER_PROTOBUF} --proto_path=${PWD}

PATCHED_OTEL_PROTO_DIR = proto-gen/.patched-otel-proto

# The source directory for OTLP Protobufs from the sub-sub-module.
OTEL_PROTO_SRC_DIR=opentelemetry-proto/opentelemetry/proto

# Find all OTEL .proto files, remove leading path (only keep relevant namespace dirs).
OTEL_PROTO_FILES=$(subst $(OTEL_PROTO_SRC_DIR)/,,\
$(shell ls $(OTEL_PROTO_SRC_DIR)/{common,resource,trace}/v1/*.proto))

# Macro to execute a command passed as argument.
# DO NOT DELETE EMPTY LINE at the end of the macro, it's required to separate commands.
define exec-command
$(1)

endef

# DO NOT DELETE EMPTY LINE at the end of the macro, it's required to separate commands.
define print_caption
@echo "🏗️ "
@echo "🏗️ " $1
@echo "🏗️ "

endef

# Macro to compile Protobuf $(2) into directory $(1). $(3) can provide additional flags.
# DO NOT DELETE EMPTY LINE at the end of the macro, it's required to separate commands.
# Arguments:
# $(1) - output directory
# $(2) - path to the .proto file
# $(3) - additional flags to pass to protoc, e.g. extra -Ixxx
# $(4) - additional options to pass to gogo plugin
define proto_compile
$(call print_caption, "Processing $(2) --> $(1)")

$(PROTOC) \
$(PROTO_INCLUDES) \
--gogo_out=plugins=grpc,$(strip $(4)),$(PROTO_GOGO_MAPPINGS):$(PWD)/$(strip $(1)) \
$(3) $(2)

endef

.PHONY: new-proto
new-proto: new-proto-api-v2
Copy link
Member

Choose a reason for hiding this comment

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

Why "new"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurishkuro
There is already a command make proto in the main Makefile. I didn't want to remove that right now before confirming, so I made the make new-proto command in the meantime.
If we can remove the old make proto command, we can rename it.

Copy link
Member

Choose a reason for hiding this comment

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

why do we need two different paths at all? The difference between the two commands is that the old one generates for all languages (and is used mostly as a linter step), while the new one only generates Go types. I would prefer to avoid duplication and be able to support both Go and All languages with a single set of commands.

Copy link
Member

Choose a reason for hiding this comment

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

you know, let's hold off on this topic - we can refactor this as as separate issue, I would like to get api_v2 types into this repo first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we need two different paths at all?

Yes, I agree. I had planned on replacing that test in the end. Just wanted to show you the changes before making them; hence, I made a new file.

Okay, I will create another pr to make these changes


.PHONY: new-proto-api-v2
new-proto-api-v2:
mkdir -p proto-gen/api_v2
$(call proto_compile, proto-gen/api_v2, proto/api_v2/query.proto)
$(call proto_compile, proto-gen/api_v2, proto/api_v2/collector.proto)
$(call proto_compile, proto-gen/api_v2, proto/api_v2/sampling.proto)
Loading
Loading