[chore] fix check-codeowners; add components for otelcol and confighttp#13951
[chore] fix check-codeowners; add components for otelcol and confighttp#13951axw wants to merge 3 commits into
Conversation
Add a component for otelcol, and confighttp. The former is for open-telemetry#13950, while the latter is needed to fix .chloggen/13783.yaml.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13951 +/- ##
==========================================
- Coverage 91.67% 91.64% -0.04%
==========================================
Files 653 653
Lines 42554 42554
==========================================
- Hits 39013 38997 -16
- Misses 2733 2745 +12
- Partials 808 812 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Ah, another tools leftover: cc: @codeboten , @mx-psi : will submit a PR to get this one fixed asap. EDIT: Just noticed, it got fixed by @axw in 48100ee#diff-33e518bda3b8106fe0cb33b8ed49e65aa45b7c9ffd07efcb06e9bc00b575ab17R60 IMO, calling out to |
Agreed. The existing Makefile target is slightly different (it passes |
| run: | | ||
| cd pr | ||
| GITHUB_TOKEN=${{ steps.otelbot-token.outputs.token }} ../.tools/githubgen --default-codeowner "open-telemetry/collector-approvers" codeowners | ||
| GITHUB_TOKEN=${{ steps.otelbot-token.outputs.token }} go tool -modfile=../internal/tools/go.mod githubgen --default-codeowner "open-telemetry/collector-approvers" codeowners |
There was a problem hiding this comment.
Having a separate Makefile target for this one would be better, IMHO.
Or perhaps the existing generate-codeowners may be adapted to expect additional args (such as -skipgithub, etc.), so it can be re-used?
opentelemetry-collector/Makefile
Lines 469 to 471 in 0cdc78b
There was a problem hiding this comment.
Yeah I was being a little bit lazy. I'll add an args env var for the target and use that.
There was a problem hiding this comment.
Wouldn't it make more sense to verify whether we are running in CI or not, and then set opts for codeowner generation respectively?
For example, this would not need to change the existing workflow.
WDYT?
diff --git a/Makefile b/Makefile
index 3ce7d1c98..a0bc35971 100644
--- a/Makefile
+++ b/Makefile
@@ -335,11 +335,13 @@ checkdoc:
apidiff-build:
@$(foreach pkg,$(ALL_PKGS),$(call exec-command,./internal/buildscripts/gen-apidiff.sh -p $(pkg)))
-# If we are running in CI, change input directory
+# If we are running in CI, change input directory. Also configure opts for generating codeowners
ifeq ($(CI), true)
APICOMPARE_OPTS=$(COMPARE_OPTS)
+GENCODEOWNERS_OPTS=
else
APICOMPARE_OPTS=-d "./internal/data/apidiff"
+GENCODEOWNERS_OPTS=-skipgithub
endif
# Compare API state snapshots
@@ -468,7 +470,7 @@ generate-gh-issue-templates:
.PHONY: generate-codeowners
generate-codeowners:
- $(GO_TOOL) githubgen --default-codeowner "open-telemetry/collector-approvers" -skipgithub codeowners
+ $(GO_TOOL) githubgen --default-codeowner "open-telemetry/collector-approvers" $(GENCODEOWNERS_OPTS) codeowners
.PHONY: gengithub
gengithub: generate-codeowners generate-gh-issue-templatesThere was a problem hiding this comment.
I'm not so sure about that, for two reasons:
- we need to pass in
-folderto make it look in the PR branch checkout - "running in CI" isn't quite enough I think, or at least could be brittle: in this case we can check codeowners against the GitHub API because of the elevated access, so it's not just a matter of checking that we're in CI, but knowing that it's running with a GITHUB_TOKEN with additional access
|
The confighttp changelog entry has been fixed in another PR, and the codeowners one is unrelated to adding an otelcol component. I'll separate that out. |
Description
Add components for otelcol and confighttp. The former is for #13950, while the latter is needed to fix .chloggen/13783.yaml.
Link to tracking issue
N/A
Testing
N/A
Documentation
N/A