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.

Sign up for GitHub

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

Jump to bottom

fix: post-process cfn files for minification, consolidating concern of minification a bit #12072

Merged
merged 4 commits into from
Feb 22, 2023
Merged

fix: post-process cfn files for minification, consolidating concern of minification a bit #12072

merged 4 commits into from
Feb 22, 2023

Conversation

alharris-at
Copy link
Contributor

@alharris-at alharris-at commented Feb 22, 2023 •
edited
Loading

Description of changes

When the --minify flag is passed in, post-process generated CFN files to read them back into memory, and rewrite in a shortened form. This will allow us to remove minify handling from the various places it exists today elsewhere in code, and have a single (or small number) of places where the Amplify CLI handles this.

Issue #, if available

#7680

Description of how you validated changes

Added unit tests, and verified manually via amplify-dev that that w/ and w/o --minify flag behaves as expected for both standard and iterative deployment use-cases (previous attempts to resolve this haven't resolved iterative deployments, leaving customers in a state where initial deploy succeeds, and adding/editing relationships will fail).

Added an e2e test that verifies that both my nested stack files and top-level stack files will be emitted as minified: https://app.circleci.com/pipelines/github/aws-amplify/amplify-cli/16432/workflows/26a70f8c-a154-4ed0-a4f4-5573f5fe67d2

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@alharris-at alharris-at requested a review from a team as a code owner February 22, 2023 00:57
@alharris-at alharris-at force-pushed the consolidate-minify-handling branch from dea73f4 to 203a46e Compare February 22, 2023 01:02
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 22, 2023
View reviewed changes
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-utils.test.ts Fixed Show fixed Hide fixed
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-utils.test.ts Fixed Show fixed Hide fixed
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-utils.test.ts Fixed Show fixed Hide fixed
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-utils.test.ts Fixed Show fixed Hide fixed
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-utils.test.ts Fixed Show fixed Hide fixed
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-utils.test.ts Fixed Show fixed Hide fixed
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-utils.test.ts Fixed Show fixed Hide fixed
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-utils.test.ts Fixed Show fixed Hide fixed
packages/amplify-provider-awscloudformation/src/utils/minify-utils.ts Fixed Show fixed Hide fixed
sundersc
sundersc previously approved these changes Feb 22, 2023
View reviewed changes
@alharris-at alharris-at force-pushed the consolidate-minify-handling branch from 203a46e to e49880b Compare February 22, 2023 18:16
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 22, 2023
View reviewed changes
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts
describe('minifyJSONFile', () => {
const writeFileMinifyAndReturnReadContents = (filePath: string, contents: string): string => {
const tmpFilePath = path.join(os.tmpdir(), filePath);
fs.writeFileSync(tmpFilePath, contents);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can this warning be fixed?

packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts
fs.mkdirSync(subNestedStacksDirectory);

// Write files
fs.writeFileSync(path.join(testDirectory, 'file.json'), JSON_STRING);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1).
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts

// Write files
fs.writeFileSync(path.join(testDirectory, 'file.json'), JSON_STRING);
fs.writeFileSync(path.join(testDirectory, 'config.env'), NON_JSON_STRING);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1).
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts
// Write files
fs.writeFileSync(path.join(testDirectory, 'file.json'), JSON_STRING);
fs.writeFileSync(path.join(testDirectory, 'config.env'), NON_JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested_1.json'), JSON_STRING);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1).
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts
fs.writeFileSync(path.join(testDirectory, 'file.json'), JSON_STRING);
fs.writeFileSync(path.join(testDirectory, 'config.env'), NON_JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested_1.json'), JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested_2.json'), JSON_STRING);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1).
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts
fs.writeFileSync(path.join(testDirectory, 'config.env'), NON_JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested_1.json'), JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested_2.json'), JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested.env'), NON_JSON_STRING);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1).
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested_1.json'), JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested_2.json'), JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested.env'), NON_JSON_STRING);
fs.writeFileSync(path.join(subNestedStacksDirectory, 'sub_nested.json'), JSON_STRING);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1).
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested_2.json'), JSON_STRING);
fs.writeFileSync(path.join(nestedStacksDirectory, 'nested.env'), NON_JSON_STRING);
fs.writeFileSync(path.join(subNestedStacksDirectory, 'sub_nested.json'), JSON_STRING);
fs.writeFileSync(path.join(subNestedStacksDirectory, 'sub_nested.env'), NON_JSON_STRING);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1).
packages/amplify-provider-awscloudformation/src/utils/minify-json.ts
if (!jsonFilePath.includes('.json')) return; // Don't convert files not ending in `.json`
const originalJSON = fs.readFileSync(jsonFilePath, 'utf-8');
const minifiedJSON = JSON.stringify(JSON.parse(originalJSON));
fs.writeFileSync(jsonFilePath, minifiedJSON);

Check failure

Code scanning / CodeQL

Insecure temporary file

Insecure creation of file in [the os temp dir](1). Insecure creation of file in [the os temp dir](2).
@alharris-at alharris-at force-pushed the consolidate-minify-handling branch from e49880b to 0a2b269 Compare February 22, 2023 19:35
jhockett
jhockett previously approved these changes Feb 22, 2023
View reviewed changes
pavellazar
pavellazar reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@pavellazar pavellazar left a comment

Choose a reason for hiding this comment

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

LGTM, let a few nits, mostly concerned about the CodeQL warnings

packages/amplify-e2e-tests/src/__tests__/minify-cloudformation.test.ts Outdated Show resolved Hide resolved
packages/amplify-provider-awscloudformation/src/__tests__/utils/minify-json.test.ts
describe('minifyJSONFile', () => {
const writeFileMinifyAndReturnReadContents = (filePath: string, contents: string): string => {
const tmpFilePath = path.join(os.tmpdir(), filePath);
fs.writeFileSync(tmpFilePath, contents);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can this warning be fixed?

@pavellazar pavellazar merged commit 8297993 into dev Feb 22, 2023
@pavellazar pavellazar deleted the consolidate-minify-handling branch February 22, 2023 22:08
@alharris-at alharris-at mentioned this pull request Feb 22, 2023
Merged
2 tasks
sobolk
sobolk reviewed Feb 23, 2023
View reviewed changes
packages/amplify-e2e-core/src/init/amplifyPush.ts
@@ -29,16 +29,21 @@ export type LayerPushSettings = {
usePreviousPermissions?: boolean;
};

export type PushOpts = {
Copy link
Member

Choose a reason for hiding this comment

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

PushOptions would be much better.

Amplifiyer pushed a commit to Amplifiyer/amplify-cli that referenced this pull request Feb 23, 2023
@alharris-at @Amplifiyer
fix: post-process cfn files for minification, consolidating concern o…
2b7bd72 
…f minification a bit (aws-amplify#12072)

* fix: post-process cfn files for minification, consolidating concern of minification a bit

* chore: applying prettier

* fix: update to use amplifyPushUpdate

* chore: update param style for push
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolk sobolk sobolk left review comments

@sundersc sundersc sundersc approved these changes

@pavellazar pavellazar pavellazar approved these changes

@jhockett jhockett jhockett left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alharris-at @jhockett @sobolk @sundersc @pavellazar