Introducing contrib builds #33
Conversation
owais
requested review from
bogdandrutu,
flands,
pjanotti,
rghetia,
songy23 and
tigrannajaryan
as code owners
owais
force-pushed
the
prepare-contrib-builds
branch
2 times, most recently
from
5c27f89
to
e8e388e
Compare
owais
force-pushed
the
prepare-contrib-builds
branch
from
e8e388e
to
3b3138e
Compare
cmd/otelcontribcol/Dockerfile
Outdated
|
|
||
| FROM scratch | ||
| COPY --from=certs /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt | ||
| COPY otelcontribcol /usr/bin/ |
There was a problem hiding this comment.
nit: we put OTelCol straight on the root, we should follow a consistent pattern between these two. it can be either one but it should be consistent.
cmd/otelcontribcol/components.go
Outdated
| errs := []error{} | ||
| factories, err := defaults.Components() | ||
| if err != nil { | ||
| return factories, err |
There was a problem hiding this comment.
nit:
| return factories, err | |
| return nil, err |
| @@ -5,6 +5,9 @@ go 1.12 | |||
| require ( | |||
| github.com/client9/misspell v0.3.4 | |||
| github.com/google/addlicense v0.0.0-20190907113143-be125746c2c4 | |||
| github.com/open-telemetry/opentelemetry-collector v0.2.1-0.20191016224815-dfabfb0c1d1e | |||
There was a problem hiding this comment.
Can we have this module together with main.go? This will follow the same pattern that we have for individual components.
There was a problem hiding this comment.
Good point. I was planning on doing it if/when we introduce multiple builds from this repo (protobuf vs gogoproto) but will look into doing it right away. We'll have to keep this go.mod around anyway so this repo can be consumed as a lib easily (stuff other than explicit modules like receivers, exporters).
There was a problem hiding this comment.
On second thought, I think we should punt it for now. If we make it a module, it'll not be able to access any internal package from the rest of the repo. The cmd module will by default import code from Github master instead of the local code so local development will be annoying. We can add permanent replace directives but I think there will be more issues. We'll mostly need to update the Makefile, CI and some scritps to supports it. Added a ticket to track it: #35
| "github.com/open-telemetry/opentelemetry-collector/receiver" | ||
|
|
||
| "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/stackdriverexporter" | ||
| "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/zipkinscribereceiver" |
There was a problem hiding this comment.
OK. Both of these components have tests for default config and for factory. This should be our minimum bar for accepting components into the build. We can think about having stricter requirements but I believe that's bare minimum.
There was a problem hiding this comment.
Created #34 to make such requirements explicit.
Makefile
Outdated
| .PHONY: addlicense-fmt-vet-lint-goimports-misspell-staticcheck-test | ||
| addlicense-fmt-vet-lint-goimports-misspell-staticcheck-test: addlicense fmt vet lint goimports misspell staticcheck test | ||
|
|
||
| .PHONY: e2e-test |
There was a problem hiding this comment.
I suggest to remove from makefile for now and add with testbed.
There was a problem hiding this comment.
Do you plan to remove this from this PR?
There was a problem hiding this comment.
I thought I did. Checing
There was a problem hiding this comment.
Looks like the push go through for some reason.
|
Thanks for the review @tigrannajaryan @pjanotti and it was closer to 30 mins :) A little over may be. |
|
Of course including the testbed does not count :D |
owais
force-pushed
the
prepare-contrib-builds
branch
from
3b3138e
to
8a7b2c5
Compare
|
Updated PR |
Added a makefile with targets to allow building the contrib distribution. The contrib distribution includes all components present in the upstream distribution plus all components present in the contrib repo. - Does not include the e2e testbed yet. I'll add that in a follow up PR. - Also copies version package from upstream. Need to re-use that. Fixes #17
owais
force-pushed
the
prepare-contrib-builds
branch
from
8a7b2c5
to
cd72720
Compare
Since these methods are about the span itself rather than specifically *events* on the span, it makes sense to drop "events" from their names. [Closes #33]
Retry IMDSV2 Before Falling Back To IMDSV1
Added a makefile with targets to allow building the contrib
distribution.
The contrib distribution includes all components present in the upstream
distribution plus all components present in the contrib repo.