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

Support Platform CLI #763

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

Support Platform CLI #763

wants to merge 67 commits into from

Conversation

lorenyu
Copy link
Contributor

@lorenyu lorenyu commented Oct 4, 2024

Ticket

Resolves #691

Changes

A new tool, the Platform CLI[1], has been developed to replace the existing install/update scripts. It currently utilizes the copier[2] library to drive most of the logic. Copier by design really prefers one-repo = one-template, which is not quite how we have template-infra setup. So the Platform CLI has special logic to treat this repo as effectively two different templates, "base" and "app":

  • base: all non-app-specific code
  • app: basically anything with {{app_name}} in the file path

To support this new tool, a variety of changes are necessary, notable ones being:

  • Add a copier.yml config file which holds the questions for templating, with only questions relevant to "base" or "app" templates being asked and saved during the relevant install/update phases
    • Most of project-config is now configuration on the "base" template
    • Apps can consistently enable CI/CD things by updating app_has_dev_env_setup answer, rather than manually finding and un-commenting bits of code
  • Add a .template-infra/ folder to hold the answers files that copier generates in projects
  • Templatize app_name to be dynamically generated on install for naming files with {{app_name}}
  • Update .github/workflows/ci-infra-service.yml (and associated infra/test/infra_test.go) to become parameterized/setup per-app (in ci-{{app_name}}-infra-service.yml.jinja)
  • Update infra/test/infra_test.go
    • Paramaterize app name based on env var (supporting previous item)
    • Use the /health endpoint instead of / for check since all apps are required to have a health endpoint but not required to respond to a root request
    • Disable verifying TLS certs on the health request to work around issue for apps using enable_https.
  • Update various docs that used to link to a infra/app to now reference infra/<APP_NAME> (and normalize most of the docs to use <APP_NAME> as the placeholder for this, instead of the mix of things we were using before)
  • Move app/ to template-only-app/. Places wanting to use it as their example app for testing can copy template-only-app/ to the appropriate app-named directory.
  • Update .github/workflows/template-only-ci-infra.yml to use the Platform CLI to install the template with example app and run tests against that. Necessitates the template_infra_test.go changes.
  • The Platform CLI has special logic to re-render infra/networks/main.tf.jinja with the installed apps
  • Update PR Environments message logic to preserve and cleanup info for multiple apps.
  • Remove old install/update scripts

[1] https://github.com/navapbc/platform-cli
[2] https://copier.readthedocs.io/en/stable/

Context for reviewers

TODO:

  • Maybe rename all the "base" specific template questions to base_<foo>? Then the CLI tool could read the base answers and auto populate them for "app" stuff? Ideally we'd separate the base and app infra templates to make this cleaner, but in the interim maybe do that? Move "app" questions to an app_ prefix?
  • Rename app/. Maybe example-app/ or template-only-app/
  • update the workflow links in docs (.github/workflows/README.md, docs/infra/pull-request-environments.md) to point to blob/main before merge

The Template CI Infra Checks / Infra Tests CI failures are due to #781, the CI was passing before the AWS changes.

Potentially resolves #447 (depending on the final decided scope for that ticket)
Potentially resolves #557
Potentially resolves #478

Resolves #569
Resolves #647

Testing

This repo's CD has been updated to use the Platform CLI tool and this branch: https://github.com/navapbc/template-infra/blob/main/.github/workflows/template-only-cd.yml

WIP on getting pfml-starter-kit-app using the Platform CLI tool and this branch: https://github.com/navapbc/pfml-starter-kit-app/pull/223

And the cleaner pfml-starter-kit-app upgrade PR: https://github.com/navapbc/pfml-starter-kit-app/pull/225

@doshitan doshitan force-pushed the lorenyu/platform-cli branch 2 times, most recently from 79fe466 to 38922e1 Compare November 5, 2024 23:06
@doshitan doshitan force-pushed the lorenyu/platform-cli branch from 38922e1 to b6f626e Compare November 5, 2024 23:10
* main:
  Update obsolete GitHub Actions (#774)
@lorenyu
Copy link
Contributor Author

lorenyu commented Nov 6, 2024

@doshitan I see you merged main into this branch, but I was wondering if that's necessary. template-only-cd is supposed to do that automatically now, is that step not working as intended?

@doshitan
Copy link
Contributor

doshitan commented Nov 6, 2024

@doshitan I see you merged main into this branch, but I was wondering if that's necessary. template-only-cd is supposed to do that automatically now, is that step not working as intended?

There was a conflict on the auto merge: https://github.com/navapbc/template-infra/actions/runs/11705178511/job/32599329386

@doshitan doshitan force-pushed the lorenyu/platform-cli branch from 0125619 to 25904e1 Compare November 6, 2024 23:14
doshitan and others added 30 commits November 12, 2024 13:40
Once this PR is merged, things should use `main`/whatever branch it's
running from.
Tracked as TODO on the PR
Since platform-cli defaults to `lorenyu/platform-cli` branch still.
This should keep things working through the transition period.
* main:
  Deploy to pfml-starter-kit-app as a part of CD
By specifying `_exclude` in `copier.yml`, we need to re-add the relevant
default excludes to the list (`.git` and `copier.yml`).

https://copier.readthedocs.io/en/stable/configuring/#exclude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment