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: Manage grafana content/assets via terraform #913

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

mrnicegyu11
Copy link
Member

@mrnicegyu11 mrnicegyu11 commented Dec 16, 2024

What do these changes do?

  • Add: Conveniant access to CI_ENV_FILE to common.Makefile (necessary for handling S3 tf-state in grafana-tf)
  • Remove: grafana-export Makefile target, old python grafana import/export scripts
  • Add: terraform for grafana dashboard management, associated Makefile, jinja2 templating for terraform-grafana to make local deployment with local file provider for terraform state work

Related issue/s

#778

Related PR/s

https://git.speag.com/oSparc/osparc-ops-deployment-configuration/-/merge_requests/1170

Checklist

  • I tested and it works

@mrnicegyu11 mrnicegyu11 self-assigned this Dec 16, 2024
@@ -0,0 +1 @@
1.5.1
Copy link
Member Author

Choose a reason for hiding this comment

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

is this the right place for the .terraform-version file? Should we rather have only one tf version for the whole repo, at the repo base dir, and maybe even have a pre-commit hook that enforces only one single tf version? not sure

url = var.prometheus_catchall_url
basic_auth_enabled = false
is_default = false
uid = "RmZEr52nz"
Copy link
Member Author

@mrnicegyu11 mrnicegyu11 Dec 16, 2024

Choose a reason for hiding this comment

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

This "could" have been a password (the academic proper term is "high entropy string" I guess). I think we should have some sort of test (pre-commit?) that asserts that no high-entropy strings are commited in this repo, as it is public on github, and we might leak secrets, and rotating secrets is a lot of annoying work, so I'd like that not to happen :D

@mrnicegyu11 mrnicegyu11 mentioned this pull request Dec 18, 2024
1 task
@mrnicegyu11 mrnicegyu11 changed the title 2024/add/grafana terraform ✨ Add: Manage grafana content/assets via terraform Dec 18, 2024
@mrnicegyu11 mrnicegyu11 marked this pull request as ready for review December 18, 2024 09:06
Copy link
Collaborator

@YuryHrytsuk YuryHrytsuk left a comment

Choose a reason for hiding this comment

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

Thanks.

I wonder what can be done with ci.env file

# Extract DEPLOYMENT_FQDN using Make functions
DEPLOYMENT_FQDN := $(notdir $(patsubst %/,%, $(dir $(REPO_CONFIG_LOCATION))))
# Construct CI_ENV_FILE using Make functions
CI_ENV_FILE := $(realpath $(dir $(REPO_CONFIG_LOCATION))/../../.gitlab/pipelines/1_configurations/$(DEPLOYMENT_FQDN)/ci.env)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if hardcoding assumptions about the other repo here is sustainable.

We can brainstorm together what can be done and if we can avoid using ci.env file here

DEPLOYMENT_FQDN := $(notdir $(patsubst %/,%, $(dir $(REPO_CONFIG_LOCATION))))
# Construct CI_ENV_FILE using Make functions
CI_ENV_FILE := $(realpath $(dir $(REPO_CONFIG_LOCATION))/../../.gitlab/pipelines/1_configurations/$(DEPLOYMENT_FQDN)/ci.env)
$(if $(CI_ENV_FILE),,$(error The location of the repo.config file given in .config.location is invalid. Cannot find the ci.env file. Aborting))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not going to work in CI where we do not have ci.env file

@@ -253,13 +260,13 @@ venv: $(REPO_BASE_DIR)/.venv/bin/activate ## Creates a python virtual environmen
ifeq ($(shell test -f j2cli_customization.py && echo -n yes),yes)

define jinja
$(REPO_BASE_DIR)/.venv/bin/j2 --format=env $(1) .env -o $(2) --customize j2cli_customization.py
$(REPO_BASE_DIR)/.venv/bin/j2 --format=env $(1) $(2) -o $(3) --customize j2cli_customization.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if for a single corner case we need to repeat .env call in all places except the corner case

Ideas:

  1. use default value .env if only 2 arguments are passed
  2. define special / different jinja function for grafana terraform
Suggested change
$(REPO_BASE_DIR)/.venv/bin/j2 --format=env $(1) $(2) -o $(3) --customize j2cli_customization.py
$(REPO_BASE_DIR)/.venv/bin/j2 --format=env $(1) $(if $(strip $(3)),$(2),.env) -o $(if $(strip $(3)),$(3),$(2)) --customize j2cli_customization.py

set +o allexport; \
url=$${TF_VAR_GRAFANA_URL}; \
echo "Waiting for grafana at $$url to become reachable..."; \
while true; do \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we add a timeout here to avoid waiting forever?

@@ -0,0 +1,35 @@
// Import subfolders using an external script
data "external" "subfolders" {
program = ["bash", "${path.module}/../../../../scripts/tf_helper_list_subfolders.sh", "${path.module}/../assets/shared/dashboards"]
Copy link
Collaborator

@YuryHrytsuk YuryHrytsuk Dec 18, 2024

Choose a reason for hiding this comment

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

Q

since grafana dashboards are now controlled by terraform, shall we store them directly in terraform? Thus we can also avoid any relative ../ imports

data "external" "dashboard_files" {
for_each = local.folder_map

program = ["bash", "${path.module}/../../../../scripts/tf_helper_list_json_files_in_folder.sh", "${path.module}/../assets/shared/dashboards/${each.key}"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we can keep this scripts inside terraform folder to avoid having ../../../../

terraform/main.tf: terraform/main.tf.j2 .venv $(CI_ENV_FILE)
# generate $@
@$(call jinja, $<, $(CI_ENV_FILE), $@)
# validate and format $@
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it a redundant line?

@@ -0,0 +1,31 @@
terraform {
required_version = "~> 1.5.1"
{% if TF_STATE_BACKEND_TYPE != "s3" %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

stricter check? there are many terraform backend types

@@ -0,0 +1,6 @@
main.tf
.terraform/** */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: this */ is to ignore all files but it does not embrace hidden files, correct?

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.

2 participants