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

Sites.yaml: source of truth and driver of infra #246

Merged
merged 18 commits into from
Apr 10, 2024

Conversation

hypesystem
Copy link
Contributor

@hypesystem hypesystem commented Mar 12, 2024

What does this PR do?

Simplifies infrastructure alignment with sites.yaml significantly by replacing several runbooks with a single sites:sync task, which ensures that all sites are in correct state as described in sites.yaml.

This means less work when creating new sites, updating information, or changing templating for content in sites in the future. We give sites.yaml more power and require less manual work.

In the future we can then expand on the sites.yaml spec to drive what we need to happen in the infrastructure, in line with the principles in ADR 004: Declarative Site management but taking the principle slightly further and aligning on a single source of truth. This is covered in the new ADR 005 which is added in this PR.

Should this be tested by the reviewer and how?

For now - await my green light on actually running the code.

But please review the code and documentation changes for clarity.

Later: (DO NOT DO THIS NOW)

  • Run the sites:sync script and see that nothing changes
  • Make an unimportant change or add a site to sites.yaml, run task sites:sync and see that it works.

What are the relevant tickets?

DDRIFT-45

Base automatically changed from staging-envs-for-customizable-plan to add-canary-project March 14, 2024 10:29
Base automatically changed from add-canary-project to main March 14, 2024 10:29
@hypesystem hypesystem force-pushed the sites.yaml-source-of-truth branch from 57bd5de to 961f7f1 Compare March 30, 2024 19:12
@hypesystem hypesystem marked this pull request as ready for review April 4, 2024 07:32
@hypesystem hypesystem mentioned this pull request Apr 4, 2024

The following describes a semi-automated version of "Add a Project" in
[the official documentation](https://docs.lagoon.sh/installing-lagoon/add-project/).
From within `dplsh` run the `sites:sync` task to sync the site state in
Copy link
Contributor

Choose a reason for hiding this comment

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

So the only Prerequisity here would be that you are allowed to push (via an ssh key) to the dpl-platform repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it is mentioned later under "Synchronize site state for all sites" that you also need an ssh-key added via the Lagoon UI. I could maybe merge the sections now that I've rewritten much of it?

Comment on lines +35 to +41
# 3a. For project-level variables, use the following command, which creates
# the variable if it does not yet exist, or updates it otherwise:
$ task lagoon:ensure:environment-variable \
PROJECT_NAME=<project name> \
VARIABLE_SCOPE=RUNTIME \
VARIABLE_NAME=<your variable name> \
VARIABLE_VALUE=<your variable value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice with an example of a runtime/environment level variable that you could set

Copy link
Contributor Author

@hypesystem hypesystem Apr 10, 2024

Choose a reason for hiding this comment

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

Hmm... you can kind of set anything you want, it is just passed as an environment variable to the cli and php services.

I'm not sure if we have any good examples where we modify behavior based on env vars -- but it would be an obvious way to differentiate plans, if we need to, for example.

Comment on lines +530 to +545
preconditions:
- sh: "[ ! -z {{.VARIABLE_VALUE}} ]"
msg: "Missing VARIABLE_VALUE"
- sh: "[ ! -z {{.VARIABLE_NAME}} ]"
msg: "Missing VARIABLE_NAME"
- sh: "[ ! -z {{.VARIABLE_SCOPE}} ]"
msg: "Missing VARIABLE_SCOPE"
- sh: "[ ! -z {{.PROJECT_NAME}} ]"
msg: "Missing PROJECT_NAME"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the latest versions of Task you can use the requires property instead. I think it is a bit more clean:

https://github.com/danskernesdigitalebibliotek/dpl-cms/blob/9716206769278d6dc6781fe554b9a94f7a93e9f8/task/Taskfile.translations.yml#L37-L38

infrastructure/Taskfile.yml Outdated Show resolved Hide resolved
Comment on lines +830 to +839
preconditions:
- sh: "[ ! -z {{.SITE}} ]"
msg: "Env variable SITE is not set or empty."
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use requires here (and in multiple other tasks)....

infrastructure/Taskfile.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@spaceo spaceo left a comment

Choose a reason for hiding this comment

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

Phew. A lot of changes to wrap my head around.
It would make more sense to me to try it out to fully understand it, but that is a luxury that I do not have.
Please look at my remarks :)

@hypesystem hypesystem force-pushed the sites.yaml-source-of-truth branch from f278242 to 0ac2aba Compare April 10, 2024 11:35
@hypesystem
Copy link
Contributor Author

In the name of getting things out I'll push these to later PRs:

  • extract long task steps to .sh files
  • use requires instead of manually checking in preconditions

The new task `sites:sync` syncs *all* sites from sites.yaml, ensuring that everything is set up for them as it should.
This includes setting up Lagoon with GITHUB token, capturing the Lagoon deploy key, saving it in sites.yaml, and ensuring all the Github repos are provisioned with Terraform.

A single case is missing right now, which is running a first `lagoon deploy` for new sites. We need to figure out if that is actually required, and if so, if we can detect whether it needs doing
…e on environment variables

This avoids the issue of setting github credentials for already existing sites failing, because we were using a create-only method.
So now we update setting github token to use this new method and can generalize when we set the token
The runbook is superceded by the new runbook for `add-library-site-to-platform`.
…ting method

When setting environment variables in Lagoon the new method has these advantages:
- Does not fail when variable already exists, so learning to use this method means it will be easier to write Taskfile tasks with this knowledge
- Does not require looking up IDs before sending the command - it takes the project and environment names we already use to refer to these entities, and so provides a more natural interface.
@hypesystem hypesystem force-pushed the sites.yaml-source-of-truth branch from dbdb06b to c3cc0f8 Compare April 10, 2024 12:44
yq defaults to adding "!!merge" as a prefix to the now non-standard `<<:` key. This left terraform yamldecode unable to parse the file. The added code ensures we do not set the "!!merge" prefix.
They are a known issue and will be fixed soon. Right now the errors have no effect on the correctness of the deployment
@spaceo spaceo self-requested a review April 10, 2024 14:07
Copy link
Contributor

@spaceo spaceo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hypesystem hypesystem merged commit 1e4c11f into main Apr 10, 2024
1 check passed
@hypesystem hypesystem deleted the sites.yaml-source-of-truth branch April 10, 2024 14:13
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