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

Feat(site launch): support for multiple sites #665

Merged
merged 15 commits into from
Apr 6, 2023

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Mar 27, 2023

Problem

The Site Launch form returns an email per site. Ie when using the form to launch 5 sites, the cms backend will return 5 different emails. This can be a hassle for Ops as they have to keep sieving through each of the email to get the site details.
This PR modifies the site launch logic such that the DNS details of the site launches are consolidated into at most 2 emails (one for successful launches and one for unsuccessful launches)

Other sub-issues closed:
The current code in redirection domain lambda is wrong since

  1. The env var was not imported properly
  2. The call octokit.request() doesn't return an 404 when a file is not found. Instead, it throws an error, which caused our redirection lambda to crash.

Solution

Rather than sending an email directly to Ops per site launch, now only two emails are sent back to ops. (one for success, and another for failure)

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible

Tests

To just test the functionality of redirection domain lambda:

  1. Go to the lambda here. (Note: I have already deployed this version using the npm run deploy:dev -- --stage kishore-test command)
  2. Click on the event name testEvent.
  3. Run test, notice the lack of any failures.

You can change the test object to:
{ "primaryDomainSource": "<some-other-url>.isomer.gov.sg", "redirectionDomain": [ { "source": "www.<some-other-url>.isomer.gov.sg", "target": "18.136.36.203", "type": "A" } ] }, as long as the .conf file never existed in our branch, it should be updated here. This branch is being used for testing anyways, feel free to test with whatever values you like.

Here is a short video I made to show a trivial testing of the current state of site the site launch form:

video1052080339.mp4

To replicate the example shown in the video, one would need to

  1. Change the SITE_LAUNCH_FORM_KEY in your .env to the one in 1password for Form Secret Key Dev Site Launch
  2. In the env OUTGOING_QUEUE_URL, replace outgoingQueue to outgoingQueueKishoreTest
  3. Change NODE_ENV="LOCAL_DEV" (will be tackled in a separate pr)
  4. Populate your local DB for sites, repos and deployments
  5. Run ngrok http 8081
  6. Log into formSG and modify the webhook URL to the one corresponding to ngrok's url.
  7. Populate form with relevant values, then click on submit, see the email output in your local computer
  8. CLEAN UP by removing domain associations in amplify, remove any new rows created in the launches and redirections table.

Deploy Notes

This PR will have conflicts with the recent changes in node var, and the changes with convict PR. These will be addressed in a separate PR, when the changes in identity gets merged into develop. Issues have been raised here and here.

New Env Vars

SITE_LAUNCH_FORM_KEY -> use updated value in 1pw

Review Notes

I feel weird about the naming convention for this function, open to suggestions for this!

@seaerchin
Copy link
Contributor

marked as draft btw

@kishore03109 kishore03109 marked this pull request as ready for review March 27, 2023 06:01
@kishore03109 kishore03109 requested a review from a team March 27, 2023 06:02
@kishore03109 kishore03109 changed the title Feat/site launch/support for multiple sites Feat(site launch): support for multiple sites Mar 27, 2023
src/routes/formsgSiteLaunch.ts Show resolved Hide resolved
src/routes/formsgSiteLaunch.ts Outdated Show resolved Hide resolved
src/routes/formsgSiteLaunch.ts Outdated Show resolved Hide resolved
src/routes/formsgSiteLaunch.ts Outdated Show resolved Hide resolved
src/routes/formsgSiteLaunch.ts Show resolved Hide resolved
src/services/infra/InfraService.ts Outdated Show resolved Hide resolved
src/services/infra/InfraService.ts Outdated Show resolved Hide resolved
@kishore03109 kishore03109 requested a review from seaerchin March 28, 2023 00:56
@kishore03109 kishore03109 requested a review from QiluXie March 28, 2023 03:54
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

1 question, rest seems fine

}
if (fileExists) return
await octokit.request(
Copy link
Contributor

Choose a reason for hiding this comment

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

when will we ever hit this line? fileExists is true by default and when it's false, we do an early return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1bea09a arh you are right, modified!

@kishore03109 kishore03109 requested a review from seaerchin April 5, 2023 10:04
@kishore03109 kishore03109 merged commit a6f5c16 into develop Apr 6, 2023
@mergify mergify bot deleted the feat/siteLaunch/supportForMultipleSites branch April 6, 2023 05:17
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