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

Templatize version updates #5333

Merged
merged 26 commits into from
Oct 10, 2023
Merged

Conversation

spartan0x117
Copy link
Contributor

PR Description

Adds templates of all of the files that reference the latest release directly by version and adds a script to update and generate files with the version injected in the right place.

A few alternative approaches were considered:

  1. Using gomplate. It is possible to switch to it, if desired, but requires a decent amount of working around existing elements (e.g. things like {{< relref "./_index.md" >}}).
  2. Using envsubst. This has conflicts with a few places, e.g.
    namespace: ${NAMESPACE}
    , leading to generated files that have these values replaced with empty strings, which we don't want.

If the .t files are not desired, an alternative would be to add the templates all within the tools directory and maintain a mapping of where they need to be added. The downside of this approach is that it is more hidden and likely to desync (e.g. if we move a directory to internal/, if the template exists with the generated file we don't need to do anything. Otherwise, the output mapping needs to be updated or it will fail the next time someone runs it or put something in the wrong place).

Note(s) to the reviewer

The .t files are copies of the corresponding file. Going forward, only these should be modified so the generated files are correct.

The only added code is in tools/generate-version-files.sh

PR Checklist

  • Documentation added

@mattdurham
Copy link
Collaborator

This is fantastic, thoughts on adding a test to ensure its all happy?

@tpaschalis
Copy link
Member

Thanks for taking up on this initiative, it's nice to finally not have to do this manually! 🎉

One thing I'm worried about is that people might accidentally only edit the non-template file, either due to force of habit or going to our documentation page and hitting the "Suggest an edit" button.

How hard do you think it would be to add a Github Action to guard against this?

We could reuse the ./tools/generate-version-files.sh script by either running it and verifying that nothing changed with a Git command, or make it return a special status code that could tell us if some templating took place or not.

@tpaschalis
Copy link
Member

We do something similar for the tests in our Helm chart, although here we'd need to guide people to edit the template file instead.

if ! git diff --exit-code; then
echo "Helm chart documentation is not up to date. Please run 'make generate-helm-docs' and commit changes!" >&2
exit 1

@spartan0x117
Copy link
Contributor Author

Well, the new workflow did correctly find that some of the non-template files were updated in main between me creating the branch and now 😅

-looking to use. To learn more about the custom resources Agent Operator provides and their hierarchy, see [Grafana Agent Operator architecture]({{< relref "./architecture" >}}).
+looking to use. To learn more about the custom resources Agent Operator provides and their hierarchy, see [Grafana Agent Operator architecture]({{< relref "./architecture/" >}})

@spartan0x117
Copy link
Contributor Author

I update the template files to be of the form path/to/file.t.ext rather than path/to/file.ext.t because it breaks any syntax highlighting (technically fixable in most text editors by adding custom checks, but I'd rather not force folks to do that) and therefore makes editing the templates more painful.

This way editing templates should be roughly as easy as before, except perhaps in cases where $AGENT_VERSION doesn't play nicely 😅

…still detect the right syntax"

This could cause problems, since the template files may be picked up by
other tools.

This reverts commit b6868c4.
@spartan0x117
Copy link
Contributor Author

I update the template files to be of the form path/to/file.t.ext rather than path/to/file.ext.t because it breaks any syntax highlighting (technically fixable in most text editors by adding custom checks, but I'd rather not force folks to do that) and therefore makes editing the templates more painful.

This way editing templates should be roughly as easy as before, except perhaps in cases where $AGENT_VERSION doesn't play nicely 😅

Turns out this approach may not be a good idea, since other tools may pick up those files (for example pkg/operator/defaults.t.go doesn't get along with the generated pkg/operator/defaults.go), so I reverted the commit.

@jdbaldry
Copy link
Member

jdbaldry commented Oct 4, 2023

For docs, I think we can avoid templating every file that has reference to the version by setting the version as cascading front matter in docs/sources/_index.md.

By adding the front matter:

cascade:
  AGENT_RELEASE: $AGENT_VERSION

Any child page can access that parameter at build time with the Hugo param shortcode, for example: The current Grafana Agent release version is {{< param "AGENT_RELEASE" >}}.

In this case we only template that one file and let any other page use the variable wherever they need.

@spartan0x117
Copy link
Contributor Author

@jdbaldry That's perfect! Added it and it works like a charm for all of the markdown docs 😃

@jdbaldry
Copy link
Member

jdbaldry commented Oct 5, 2023

Thanks for making those changes!

Release information is super handy to have available to the pages and minimizing the number of pages that actually have to be fully templated is also nice.

I'll let the rest of the maintainers pick up the PR for full review but I'm happy with the docs side of things for sure.

Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

The docs changes look great. I hope this reduces the effort on the run-up to each release now. Changing one place and cascading... is really useful.

Approving doc changes

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Oct 5, 2023
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM! There's a few files which shouldn't be templated, since they're already generated files.

I also found some instances where we can reduce the number of templated files by cleaning up our documentation and removing stale references to things. That would be helpful long term.

Either way, not blocking this, it's a good improvement!

@@ -0,0 +1,115 @@
apiVersion: v1
Copy link
Member

Choose a reason for hiding this comment

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

The files in production/kubernetes/*.yaml are generated from Jsonnet, so we shouldn't template these, but rather the Jsonnet that they're generated from :)

Comment on lines 1 to 34
#!/usr/bin/env bash
# shellcheck shell=bash

#
# install-bare.sh is an installer for the Agent without a ConfigMap. It is
# used during the Grafana Cloud integrations wizard and is not recommended
# to be used directly. Instead of calling this script directly, please
# make a copy of ./agent-bare.yaml and modify it for your needs.
#
# Note that agent-bare.yaml does not have a ConfigMap, so the Grafana Agent
# will not launch until one is created. For more information on setting up
# a ConfigMap, please refer to:
#
# Metrics quickstart: https://grafana.com/docs/grafana-cloud/quickstart/agent-k8s/k8s_agent_metrics/
# Logs quickstart: https://grafana.com/docs/grafana-cloud/quickstart/agent-k8s/k8s_agent_logs/
#

check_installed() {
if ! type "$1" >/dev/null 2>&1; then
echo "error: $1 not installed" >&2
exit 1
fi
}

check_installed curl
check_installed envsubst

MANIFEST_BRANCH=$AGENT_VERSION
MANIFEST_URL=${MANIFEST_URL:-https://raw.githubusercontent.com/grafana/agent/${MANIFEST_BRANCH}/production/kubernetes/agent-bare.yaml}
NAMESPACE=${NAMESPACE:-default}

export NAMESPACE

curl -fsSL "$MANIFEST_URL" | envsubst
Copy link
Member

Choose a reason for hiding this comment

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

It would be good for us to remove the production/kubernetes folder entirely, honestly. We don't maintain it anymore. I only see one remaining reference to it that uses a recent version:

https://grafana.com/docs/agent/latest/static/set-up/install/install-agent-kubernetes/

This guide can be changed to use our Helm chart instead for static mode, with some opinionated values.yaml files.

cc @clayton-cornell for a future goal

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #5380 so this doesn't get lost.

@@ -0,0 +1,645 @@
apiVersion: v1
Copy link
Member

Choose a reason for hiding this comment

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

Ditto with this file: we may be ale to delete it in favor of the operator Helm chart; I'm not sure where this is used at all, actually. I can't find any references to it in the website or in this repo, so it might be safe to remove it.

@@ -0,0 +1,142 @@
local agent = import './internal/agent.libsonnet';
Copy link
Member

Choose a reason for hiding this comment

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

Cleanup task: we can remove the production/tanka folder completely once we remove production/kubernetes

@spartan0x117 spartan0x117 merged commit bb974be into main Oct 10, 2023
7 checks passed
@spartan0x117 spartan0x117 deleted the spartan0x117/templatize-version-updates branch October 10, 2023 21:36
tpaschalis pushed a commit to tpaschalis/agent that referenced this pull request Oct 16, 2023
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants