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(amplify migration): script to perform Netlify to Amplify Migration #55

Merged
merged 40 commits into from
Mar 15, 2023

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Mar 9, 2023

Problem

Currently, in the conversion scripts, we have scripts to add a trailing slash at the end of the permalinks. The main functionality of it is to change permalink: some/path to permalink: some/path/ in our relevant .md files. More info here.

However, the creation of the application in Amplify is not automated. Additionally, the changing of URLs from capitalisation to small casing is not tackled.

TLDR, this script will

  1. Create Amplify Application
  2. Change all instances of permalink: some/LINK/with-no-slash to permalink: some/link/with-no-slash/ and store them in a list
  3. Change all instances of <a href="some/LINK/with-no-slash"></a> to <a href="some/link/with-no-slash"></a>
  4. Change all instances of [click me](some/LINK/with-no-slash) to [click me](some/link/with-no-slash)
  5. Push relevant changes to repo directly
  6. Create SQL Commands to be run directly on prod DB to populate DB with relevant values

Known limitations:

Currently, this script is using a regex pattern to parse markdown. I have added some cases that is covers below, but there might be edge cases that are not tackled. Note: I tried to use Markdown-it , but it seems to have some quirks. (eg. it changes all instances of ' to &quot and it seemed like more work to get it to work T.T)

This script not directly talking to sql database. As a result, user id is manually hardcoded, and there a need to copy paste sql commands into prod db. Since this migration is only run once per site, I think we shouldnt over-engineer an API endpoint for something that we are not maintaining in the long run. When we do introduce backend in this script, we can modify this limiation.

Testing
Since this is infrastructure changes, it is more challenging to test this and capture all edge cases (esp considering the existence of v1 sites). Here is an example of the changes that a repo would go through when being run by the script.

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
@seaerchin
Copy link
Contributor

seaerchin commented Mar 9, 2023

btw do we need to update _config.yml also if the resource room is impacted? and also take note that _data/<navigation|footer>.yml needs to be changed too

@kishore03109
Copy link
Contributor Author

btw do we need to update _config.yml also if the resource room is impacted? and also take note that _data/<navigation|footer>.yml needs to be changed too

good question! don't need to update _config.yml, since that only contains the title of the resource file, not the url
_data/navigation.yml is already covered, and footer.yml already has trailing slash by defult!

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.

we seem to be lacking a tsconfig here; can just rip from isomer.

src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
src/amplify-migration/amplifyMigrationScript.ts Outdated Show resolved Hide resolved
@kishore03109
Copy link
Contributor Author

@seaerchin I am inclined not to add a ts.config.json file for this repo.
The idea of ts.config.json is to allow the tsc to to find the root of a TypeScript project.
I see isomer-conversion-scripts as a bunch of scripts that could be used to automate certain workflows, which might have several roots depending on the intended outcome. Let me know what you think about this!

@kishore03109 kishore03109 requested a review from seaerchin March 13, 2023 07:32
@seaerchin
Copy link
Contributor

@seaerchin I am inclined not to add a ts.config.json file for this repo. The idea of ts.config.json is to allow the tsc to to find the root of a TypeScript project. I see isomer-conversion-scripts as a bunch of scripts that could be used to automate certain workflows, which might have several roots depending on the intended outcome. Let me know what you think about this!

we use tsconfig not just to specify the root of the project but also the behaviour of tsc itself.

by not specifying a tsconfig here, we're actually using the default tsconfig of ts-node (see here), which means that behaviour might be different based on eg: system default node version.

to avoid unpleasant surprises in the future, it's best to still specify. the easiest one is per the ts-node docs:

{
  "extends": "ts-node/node16/tsconfig.json",
  // Or install directly with `npm i -D @tsconfig/node16`
  "extends": "@tsconfig/node16/tsconfig.json",
}

@kishore03109
Copy link
Contributor Author

we use tsconfig not just to specify the root of the project but also the behaviour of tsc itself.

Fair point @seaerchin! added them here 363d9ab


function readCsvFile(): Promise<[string, string][]> {
// read file list-of-repos.csv
const filePath = path.join(__dirname, "list-of-repos.csv");
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why we're not letting the users specify the csv directly using the cli?

if this is only to be used by you, it's ok but otherwise is the script itself idempotent? (seems like hor so should be ok)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated here: 66708a6

otherwise is the script itself idempotent?

hmm no leh, got quite a number of side effects. (eg. creation of amplify apps, sql commands written to db). The goal of this script was never idempotency tho.

to be truly idempotent, we would need to talk to the db, which can be tackled in a seperate PR ya

@kishore03109 kishore03109 merged commit e74ad37 into staging Mar 15, 2023
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.

3 participants