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

Update documentation to clarify support for multiple AWS accounts and multiple applications #589

Draft
wants to merge 91 commits into
base: main
Choose a base branch
from

Conversation

rocketnova
Copy link
Contributor

@rocketnova rocketnova commented Apr 25, 2024

Ticket

Changes

What was added, updated, or removed in this PR.

  • Update template-only-bin scripts for updating the template when there are multiple apps
  • Add template-only-bin scripts for adding subsequent apps
  • Revise existing documentation to clarify how to add multiple apps
  • Add new documentation for multiple apps

Context for reviewers

Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers.

This PR provides a comprehensive revision for all of the setup documentation, with an emphasis on providing clear and consistent guidance on how to set up a new project with one or more applications. It addresses existing setup and update instructions that are not clear and/or have circular dependencies (e.g. should you set up network first or app first?).

The new documentation uses the same format for all set up documents:

  • Introduction
  • Prerequisites
  • Instructions
  • Numbered headings for each instruction step

All of the documentation has been revised for basic technical writing best practices (e.g. active voice over passive voice, short clear sentences, define new terms, etc).

Note: this PR does not attempt to address #581.

Testing

Provide evidence that the code works as expected. Explain what was done for testing and the results of the test plan. Include screenshots, GIF demos, shell commands or output to help show the changes working as expected. ProTip: you can drag and drop or paste images into this textbox.

Testing setup

Our current scripting process recommends that users curl from the main branch of this repo, so the changes in this PR follow that same pattern. That means that testing requires a manual (noted below).

  1. Checkout this branch (i.e. rocket/multi-app).
  2. In /template-only-bin/download-and-install-app.sh, replace main/template-only-bin with rocket/multi-app/template-only-bin.
  3. In /template-only-bin/update-template.sh, replace main/template-only-bin with rocket/multi-app/template-only-bin.

Test 1: Install into a project with a single app

  1. Clone the nextjs template into a new directory.

    git clone git@github.com:navapbc/template-application-nextjs.git test-one-app 
  2. Change into the new project directory

    cd test-one-app 
  3. Dog-food the revised documentation in this PR. Start with the main README.md.

    1. Run the normal install script to install the latest main of the infra template.
    2. Run git commit after installing and before configuring the infra template, so you can easily verify changes using git diff.
  4. Verify that the the full infra set up works as expected.

Screenshot showing a fully configured project (No gif I created fit under the github file size limit of 10MB)

CleanShot 2024-05-15 at 18 32 30@2x

Test 2: Update a project with a single app

  1. Using the test-one-app project dir from Test 1, follow the update directions.

    1. Normally, you would run the curl command, but because that runs the version of the script on main without the changes in this PR and you want to test the changes in this PR, instead run the following to update to the latest release version of the template:
    cat <PATH_TO_INFRA_TEMPLATE>/template-only-bin/update-template.sh | bash -s -- app v0.8.0 tag
  2. Read the output and run git diff to verify changes.

Example output for updating to a branch

CleanShot 2024-05-16 at 10 05 04@2x

Example output for updating to a tag

CleanShot 2024-05-16 at 10 07 03@2x

Test 3: Install into a project with multiple apps

  1. Clone the nextjs template into a new directory.

    git clone git@github.com:navapbc/template-application-nextjs.git test-multiple-apps 
  2. Change into the new project directory

    cd test-multiple-apps 
  3. Install the infra template. Run the normal install script to install the latest main of the infra template:

    curl https://raw.githubusercontent.com/navapbc/template-infra/main/template-only-bin/download-and-install-template.sh | bash -s
  4. Run git commit to commit the installed infra template.

  5. Before continuing with the remaining infra template installation steps, install a second app to the project by following these instructions.

    1. Name the application folder second-app.
    2. Normally, you would run the curl command, but because that runs the version of the script on main without the changes in this PR and you want to test the changes in this PR, instead run the following to update to the latest release version of the template:
    cat <PATH_TO_INFRA_TEMPLATE>/template-only-bin/download-and-install-app.sh | bash -s -- second-app
  6. Run git commit to commit the installed second app.

  7. Dog-food the revised documentation in this PR. Start with the main README.md. Make sure to follow the "For exach application" sections!

  8. Verify that the the full infra set up works as expected

Screenshot showing a configured account + network

CleanShot 2024-05-16 at 11 09 55@2x

Screenshot showing the first configured app CleanShot 2024-05-16 at 11 13 43@2x
Screenshot showing the second configured app

CleanShot 2024-05-16 at 11 24 13@2x

Test 4: Update a project with multiple apps

  1. Using the test-multiple-app project dir from Test 3, follow the update directions.

    1. Normally, you would run the curl command, but because that runs the version of the script on main without the changes in this PR and you want to test the changes in this PR, instead run the following to update to the latest release version of the template:
      cat <PATH_TO_INFRA_TEMPLATE>/template-only-bin/update-template.sh | bash -s -- app,second-app rocket/multi-app branch
      ```
    
  2. Read the output and run git diff to verify changes.

Example output for updating to a branch

CleanShot 2024-05-16 at 11 30 40@2x

@rocketnova
Copy link
Contributor Author

@lorenyu This PR is really close to being fully done, but I would appreciate if you could give a WIP review of this draft so we can get aligned on the high level changes.

This should be a documentation-only PR. The current draft includes changes to template-only-bin and .github that I will pull into a separate PR and then I will adjust the documentation in this PR accordingly. That means that the testing instructions in this PR description do not apply.

@rocketnova
Copy link
Contributor Author

Also, this PR does not yet incorporate recent PRs that have been merged, so I would also need to merge those changes in and update this documentation.

@rocketnova
Copy link
Contributor Author

@lorenyu Also, if it's not clear, the entrypoint to review should be:

  1. Start with: https://github.com/navapbc/template-infra/blob/rocket/multi-app/README.md
  2. Click through the links starting with the "Setup" steps

@@ -0,0 +1,38 @@
# Multiple Applications

You can use this template with multiple applications. By default, this template assumes your project has one application named `app`. However, it's straightforward to add additional applications.
Copy link
Contributor

Choose a reason for hiding this comment

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

I was actually thinking that we change install-template.sh so that it doesn't add any apps by default. Or we could call install-app at the end of install-template.

Comment on lines +36 to +38
### 4. Configure the application as usual

Follow the per-application steps in [`/infra/README.md`](/infra/README.md) to complete setup.
Copy link
Contributor

Choose a reason for hiding this comment

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

was thinking maybe we can make install-app.sh interactive where we can ask some initial configuration questions then configure the app for them. could also be a future enhancement

Comment on lines +22 to +28
sedi () {
if sed --version >/dev/null 2>&1; then
sed -i -- "$@"
else
sed -i "" "$@"
fi
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this necessary? we already use sed in the template in a lot of places and i don't recall needing this.

Comment on lines +14 to +15
# Enforce kebab-case
APP_NAME_KEBAB=$(echo "$APP_NAME" | tr "_" "-")
Copy link
Contributor

Choose a reason for hiding this comment

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

i agree the convention should be dasherized, but i don't know how i feel about silently coercing it since if we normalize here we'll have to normalize in a lot of other places, and i don't think it should be the template's responsibility to do that. if we want to prevent underscores i'd rather just error out here. or even simpler i am ok with just leaving it as a convention but not actually preventing someone from using underscores.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants