Skip to content

Feat/tf to cdk - #1425

Open
gbbayareametro wants to merge 222 commits into
mainfrom
feat/tf_to_cdk
Open

Feat/tf to cdk#1425
gbbayareametro wants to merge 222 commits into
mainfrom
feat/tf_to_cdk

Conversation

@gbbayareametro

Copy link
Copy Markdown

This PR addresses #(insert-number-here)

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

Will be updating this with the additional pipeline steps for staging and prod shortly.

I'll also be commenting code quite a bit.

How Can This Be Tested/Reviewed?

Provide instructions so we can review, including any needed configuration, and the test cases that need to be QAd.

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

@gbbayareametro gbbayareametro left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

looks good

process.env.CORS_ORIGINS || "*",
DUPLICATES_CLOSE_DATE: process.env.DUPLICATES_CLOSE_DATE || "2024-10-08 00:00 -08:00",
// Doorway has started using serviceconnect which uses TLS internally, so we can turn HTTPS_OFF on for all environments.
HTTPS_OFF: process.env.HTTPS_OFF || "true",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

note: In the new environment "HTTPS_OFF" needs to be set to false

const listingServiceUrl = runtimeConfig.getListingServiceUrl()

try {
logger.info(`requesting URL: ${listingServiceUrl}/${context.params.id}`)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

note: this is a temporary change while debugging the environment issues

Comment on lines -30 to -31
dayjs.extend(utc);
dayjs.extend(tz);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

question: is there a reason these lines were deleted? We added these to add additional options for dayjs

PGDATABASE: { value: props.databaseName || "bloom" },
CLOUDINARY_CLOUD_NAME: { value: "exygy" },
CACHE_REVALIDATE: { value: "60" },
LANGUAGES: { value: "en,es,zh,vi,tl" },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

question: why is the "Languages" and "CLOUDINARY_CLOUD_NAME" variables needed for migrations?

@YazeedLoonat YazeedLoonat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

got some questions/comments for ya

Comment thread .prettierrc
@@ -0,0 +1,6 @@
{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

question: we usually use ESLint as our linter, do we need both prettier and eslimnt?

headers,
})
} catch (e) {
console.log(e)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nitpick: I don't think we want to be logging errors like this for end users

import { Fn } from "aws-cdk-lib"
import { ISubnet, Subnet } from "aws-cdk-lib/aws-ec2"
import { Cluster, FargateService } from "aws-cdk-lib/aws-ecs"
//import * as ecs from "aws-cdk-lib/aws-ecs";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nitpick: 🧹

const pageTitle = `${listing.name} - ${t("nav.siteTitle")}`
const { profile } = useContext(AuthContext)

// logger.info(`listing name - ${listing.name}`)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nitpick: 🧹

// }

export const getStaticProps: GetStaticProps = async (context: {
export async function getServerSideProps(context: {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

question: why are we changing these? we had originally swapped this for performance gains

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@YazeedLoonat this is me trying to debug the issue in the new environment. I don't want to go to serverSideProps, but the static props is causing an issue and I'm trying to track down what is happening
https://exygy.slack.com/archives/C06USRNGSAX/p1762203262538459

export const getStaticPaths: GetStaticPaths = () => {
return { paths: [], fallback: "blocking" }
}
// export const getStaticPaths: GetStaticPaths = () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

question: same as above

Comment thread sites/public/logger.ts
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.

4 participants