`@aws-amplify/backend` should inherit version bump of same kind from dependencies #2042

ShadowCat567 · 2024-09-23T19:22:12Z

Problem

@aws-amplify/backend sometimes does not properly inherit the right kind of version bump from its dependencies.

Issue number, if available: #1693

Changes

refactored check_changeset_completeness.ts to be split into functions
added a function to check_changeset_completeness.ts that checks whether backend should be inheriting a version bump and whether it is currently inheriting the right kind of version bump
Corresponding docs PR, if applicable:

Validation

ran manual tests using different version bumps for backend and its dependencies
Test 1: backend: none, dependencies have version bump of minor

Test 2: backend: minor, dependencies are patch or none

Test 3: backend: patch, dependencies are minor or patch

Test 4: backend: patch, no dependency versions

Test 5: backend: minor, one dependency is major

Checklist

If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
If this PR requires a change to the Project Architecture README, I have included that update in this PR.
If this PR requires a docs update, I have linked to that docs PR above.
If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

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

This reverts commit 65c6794.

changeset-bot · 2024-09-23T19:22:15Z

⚠️ No Changeset found

Latest commit: c21e963

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

sobolk · 2024-09-23T20:55:50Z

scripts/check_changeset_completeness.ts

+// eslint-disable-next-line import/no-extraneous-dependencies
+import { ReleasePlan } from '@changesets/types';


we should just add this package as dev dep.

sobolk · 2024-09-23T21:08:58Z

scripts/check_changeset_completeness.ts

+  const backendName: string = '@aws-amplify/backend';
+  const versionBumpOfWrongKind: string[] = [];
+  let backendMaxVersionType: string = 'none';
+
+  for (const changeset of releasePlan.changesets) {
+    for (const release of changeset.releases) {
+      if (release.name === backendName) {
+        if (
+          release.type === 'major' ||
+          backendMaxVersionType === 'none' ||
+          (backendMaxVersionType === 'patch' && release.type === 'minor')
+        ) {
+          backendMaxVersionType = release.type;
+        }
+      }
+    }
+  }
+
+  for (const changeset of releasePlan.changesets) {
+    for (const release of changeset.releases) {
+      if (backendDependencies.includes(release.name)) {
+        if (
+          backendMaxVersionType !== release.type &&
+          (release.type === 'major' ||
+            backendMaxVersionType === 'none' ||
+            (backendMaxVersionType === 'patch' && release.type === 'minor'))
+        ) {
+          versionBumpOfWrongKind.push(release.name);
+        }
+      }
+    }
+  }
+
+  if (versionBumpOfWrongKind.length > 0) {
+    throw new Error(
+      `${backendName} has a version bump of a different kind of the following packages but is expected to have a version bump of the same kind:${EOL}${versionBumpOfWrongKind.join(
+        EOL
+      )}`
+    );
+  }


This code is a bit hard to read.

Consider adding const maxVersion = (...versionType: Array<string>): string => { // some logic to pick max }

Suggested change

const backendName: string = '@aws-amplify/backend';

const versionBumpOfWrongKind: string[] = [];

let backendMaxVersionType: string = 'none';

for (const changeset of releasePlan.changesets) {

for (const release of changeset.releases) {

if (release.name === backendName) {

if (

release.type === 'major' ||

backendMaxVersionType === 'none' ||

(backendMaxVersionType === 'patch' && release.type === 'minor')

) {

backendMaxVersionType = release.type;

}

}

}

}

for (const changeset of releasePlan.changesets) {

for (const release of changeset.releases) {

if (backendDependencies.includes(release.name)) {

if (

backendMaxVersionType !== release.type &&

(release.type === 'major' ||

backendMaxVersionType === 'none' ||

(backendMaxVersionType === 'patch' && release.type === 'minor'))

) {

versionBumpOfWrongKind.push(release.name);

}

}

}

}

if (versionBumpOfWrongKind.length > 0) {

throw new Error(

`${backendName} has a version bump of a different kind of the following packages but is expected to have a version bump of the same kind:${EOL}${versionBumpOfWrongKind.join(

EOL

)}`

);

}

const backendName: string = '@aws-amplify/backend';

let effectiveBackendVersionType: string = 'none';

let effectiveBackendDependenciesVersionType: string = 'none';

for (const changeset of releasePlan.changesets) {

for (const release of changeset.releases) {

if (release.name === backendName) {

effectiveBackendVersionType = maxVersion(effectiveBackendVersionType, release.type);

}

}

}

for (const changeset of releasePlan.changesets) {

for (const release of changeset.releases) {

if (backendDependencies.includes(release.name)) {

effectiveBackendDependenciesVersionType = maxVersion(effectiveBackendDependenciesVersionType, release.type);

}

}

}

if (effectiveBackendVersionType !== effectiveBackendDependenciesVersionType) {

throw new Error(

`${backendName} has a version bump of a different kind of the following packages but is expected to have a version bump of the same kind:${EOL}${versionBumpOfWrongKind.join(

EOL

)}`

);

}

You could also consider something like this.

Assuming maxVersion(...versions) and findEffectiveVersion(releasePlan, packageName

have something like that:

const backendVersion = findEffectiveVersion(releasePlan, '@aws-amplify/backend`); const backendAuthVersion = findEffectiveVersion(releasePlan, '@aws-amplify/backend-auth`); const backendDataVersion = findEffectiveVersion(releasePlan, '@aws-amplify/backend-data`); ... if (backendVersion !== maxVersion(backendAuthVersion, backendDataVersion, ...)) { throw}

And. don't worry about optimizing loops. These arrays have ~10-20 items. Ability to understand what code is doing is more important here.

Would you prefer I remove the versionBumpOfWrongKind array as well? Since with this rework, it won't have anything in it

And.. there's one interesting edge case here.

It might happen that @aws-amplify/backend will get version bump but all of backend-X will not. That should be a legit case.

I.e. the condition should be if(effectiveBackendVersionType < effectiveBackendDependenciesVersionType) { throw } (this is pesudocode).

For that you might either consider either creating some kind of comparator function for minor|major|patch|none

or create enum with numerical values like enum { MAJOR: 3, MINOR: 2... }..

enum sounds like the cleaner option. My original code was able to handle cases where backend got changes but backend-X didn't through this: backendMaxVersionType !== release.type && (release.type === 'major' || backendMaxVersionType === 'none' || (backendMaxVersionType === 'patch' && release.type === 'minor'))
but it is a fairly ugly looking set of code that is not intuitive to understand.

sobolk

Looks good.

sobolk · 2024-09-23T22:55:39Z

scripts/check_changeset_completeness.ts

-        }
-      }
-    }
+const versionTypeConverter = (version: VersionType): VersionTypeEnum => {


Suggested change

const versionTypeConverter = (version: VersionType): VersionTypeEnum => {

const convertVersionType = (version: VersionType): VersionTypeEnum => {

function name should be verb-ish.

sobolk · 2024-09-23T22:56:32Z

scripts/check_changeset_completeness.ts

+    maxVersion(
+      backendAuthVersion,
+      backendDataVersion,
+      backendFunctionVersion,
+      backendStorageVersion
+    )


Suggested change

maxVersion(

backendAuthVersion,

backendDataVersion,

backendFunctionVersion,

backendStorageVersion

)

Math.max(

backendAuthVersion,

backendDataVersion,

backendFunctionVersion,

backendStorageVersion

)

would this just work?

yep, the function is unnecessary

Vieltojarvi added 12 commits September 20, 2024 14:05

refactor check_changeset_completeness with functions

999cce0

check backend version bump attempt 1 with temp workaround

46b78db

testing

65c6794

Revert "testing"

4f13e93

This reverts commit 65c6794.

attempt 2 at checking changesets

fc70fe0

testing new attempt

8a3a5af

removed testing changesets

083b7d4

testing

1b07653

testing changes

36a0ea5

testing

7a6e499

removed unnecessary comments

d1cd124

Merge branch 'main' into backend-version

85b8096

explictly add return type to modifiedPackages function

136dc07

sobolk reviewed Sep 23, 2024

View reviewed changes

Vieltojarvi added 2 commits September 23, 2024 15:12

added changeset types to dev dep

19e6662

altered logic of checkBackendDepVersion

4702acf

sobolk reviewed Sep 23, 2024

View reviewed changes

altered naming

c21e963

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`@aws-amplify/backend` should inherit version bump of same kind from dependencies #2042

`@aws-amplify/backend` should inherit version bump of same kind from dependencies #2042

ShadowCat567 commented Sep 23, 2024 •

edited

Loading

changeset-bot bot commented Sep 23, 2024 •

edited

Loading

sobolk Sep 23, 2024

sobolk Sep 23, 2024

sobolk Sep 23, 2024

sobolk Sep 23, 2024

ShadowCat567 Sep 23, 2024 •

edited

Loading

sobolk Sep 23, 2024

sobolk Sep 23, 2024

ShadowCat567 Sep 23, 2024

sobolk left a comment

sobolk Sep 23, 2024

sobolk Sep 23, 2024

ShadowCat567 Sep 23, 2024

		// eslint-disable-next-line import/no-extraneous-dependencies
		import { ReleasePlan } from '@changesets/types';

	const versionTypeConverter = (version: VersionType): VersionTypeEnum => {
	const convertVersionType = (version: VersionType): VersionTypeEnum => {

@aws-amplify/backend should inherit version bump of same kind from dependencies #2042

Are you sure you want to change the base?

@aws-amplify/backend should inherit version bump of same kind from dependencies #2042

Conversation

ShadowCat567 commented Sep 23, 2024 • edited Loading

Problem

Changes

Validation

Checklist

changeset-bot bot commented Sep 23, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShadowCat567 Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`@aws-amplify/backend` should inherit version bump of same kind from dependencies #2042

`@aws-amplify/backend` should inherit version bump of same kind from dependencies #2042

ShadowCat567 commented Sep 23, 2024 •

edited

Loading

changeset-bot bot commented Sep 23, 2024 •

edited

Loading

ShadowCat567 Sep 23, 2024 •

edited

Loading