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 script to generate manifest with default config #4430

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

carles-grafana
Copy link
Contributor

@carles-grafana carles-grafana commented Dec 9, 2024

The script:

  • generates the manifest with default config YAML which is included in the docs as a reference.
  • runs in CI to check if the manifest needs to be updated as a result of new changes in the default config.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@carles-grafana carles-grafana force-pushed the generate-manifest-automatically branch 11 times, most recently from 885032d to a3ec5e8 Compare December 9, 2024 21:28
@carles-grafana carles-grafana changed the title Add script to generate default config for docs Add script to generate manifest with default config Dec 9, 2024
@carles-grafana carles-grafana force-pushed the generate-manifest-automatically branch 2 times, most recently from 9b2de05 to 84a7c43 Compare December 10, 2024 09:33
@carles-grafana carles-grafana marked this pull request as ready for review December 10, 2024 10:13
@carles-grafana carles-grafana force-pushed the generate-manifest-automatically branch from be4ed1d to 13633d9 Compare December 10, 2024 10:34
@@ -103,6 +106,9 @@ jobs:
- name: Build Tempo
run: make tempo

- name: generate-manifest
run: go run -v pkg/docsgen/generate_manifest.go
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a make target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

personally I'm not a fan of putting this kind of commands in the makefile as it tends to grow a lot, but I'm fine with it if it's the team convention

Copy link
Contributor

Choose a reason for hiding this comment

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

I rather use make as well. That's its main purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to Makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something to keep in mind is that every time we run make, some commands are run which add an overhead:

# Version number
VERSION=$(shell ./tools/version-tag.sh | cut -d, -f 1)

GIT_REVISION := $(shell git rev-parse --short HEAD)
GIT_BRANCH := $(shell git rev-parse --abbrev-ref HEAD)

For now this is negligible, but as the Makefile grows we are more likely to introduce significant overhead. I've seen this in my previous job where we had a giant Makefile, that's why I'm averse to it and I would rather not use make as a generic task runner.

@carles-grafana carles-grafana requested a review from mapno December 10, 2024 12:33
@carles-grafana
Copy link
Contributor Author

By managing to reuse the build cache, now the new command only takes 2 seconds, I just had the use the exact same build flags GO111MODULE=on CGO_ENABLED=0 in order to reuse the cache

The script:
- generates the manifest with default config YAML which is included in the docs as a reference.
- runs in CI to check if the manifest needs to be updated as a result of new changes in the default config.
@carles-grafana carles-grafana force-pushed the generate-manifest-automatically branch from a0ea9fa to f690f82 Compare December 11, 2024 09:47
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM!

@mapno mapno merged commit 5d9e8a0 into grafana:main Dec 11, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants