ci(repo): New CI pipeline #2331

dnys1 · 2022-11-08T23:41:17Z

Removes mono_repo dependency throughout
Adds custom workflows for Dart+Flutter packages
Adds aft generate workflows command for automatically generating workflows

Jordan-Nelson · 2022-11-09T19:59:58Z

packages/aft/README.md

+- `generate`: Generates various repo items
+  - `workflows`: Generates GitHub actions workflows for all packages in the repo


sdk should be listed under generate, right?

Suggested change

- `generate`: Generates various repo items

- `workflows`: Generates GitHub actions workflows for all packages in the repo

- `generate`: Generates various repo items

- `workflows`: Generates GitHub actions workflows for all packages in the repo

- `sdk`: Generates the AWS SDK for a given package

I marked the sdk command as hidden for right now. It only works for me.

Jordan-Nelson · 2022-11-09T20:13:57Z

packages/aft/lib/src/models.dart

+  /// Skip package checks for Flutter packages when running in CI without
+  /// Flutter, which may happen when testing Dart-only packages or specific
+  /// Dart versions.


I don't think I follow this. Would this be a dart-only package that has flutter listed as a dependency for some reason?

This is a roundabout way of saying "don't try to run flutter pub upgrade when flutter is not available"

Jordan-Nelson · 2022-11-09T20:30:31Z

packages/aft/lib/src/commands/generate/generate_workflows_command.dart

+import 'package:aft/aft.dart';
+import 'package:path/path.dart' as p;
+
+/// Command for generating associated items in the repo.


Update comment

Jordan-Nelson · 2022-11-09T20:56:40Z

packages/aft/lib/src/models.dart

  /// The package's pubspec.
  final PubspecInfo pubspecInfo;

  /// The package flavor, e.g. Dart or Flutter.
  final PackageFlavor flavor;

+  /// The integration test directory within the enclosing directory, if any
+  Directory? get integTestDirectory {


This isn't used currently, correct? Are you workflows for integ tests could be generated as well?

It's not currently used. Needed for aft test though. I can remove it here.

ragingsquirrel3 · 2022-11-11T23:13:46Z

.github/workflows/flutter_vm.yaml

+        uses: actions/checkout@93ea575cb5d8a053eaa0ac8fa3b40d7e05a33cc8 # 3.1.0
+
+      - name: Setup Flutter
+        uses: subosito/flutter-action@dbf1fa04f4d2e52c33185153d06cdb5443aa189d # 2.7.0


uber nit: isn't this 2.8.0 subosito/flutter-action@dbf1fa0?

Whoops. Updated 👍

ragingsquirrel3

Approved w some small nit/questions/callouts that are non-blocking.

One general question: When would it make sense to remove the tests from circle that are redundant here? Specifically thinking of flutter unit tests. I know you mentioned it's on the roadmap to remove circle runners when Dart impls finished but dk if we want to specifically remove the same tests being run in 2 places.

ragingsquirrel3 · 2022-11-11T23:21:19Z

.github/workflows/amplify_datastore.yaml

+      - next
+  pull_request:
+    paths:
+      - 'packages/amplify_datastore/**/*.dart'


question: Would it make sense to also include interface paths to this pattern so that changes to the interface execute these tests?

Yes, in the future this will be possible to do, specifically when #2068 is merged since it has the semantic understanding of the repo (which packages depend on which).

ragingsquirrel3 · 2022-11-11T23:24:15Z

.github/workflows/amplify_api.yaml

+      - next
+  pull_request:
+    paths:
+      - 'packages/api/amplify_api/**/*.dart'


question: Is the reason for having the category flutter vm tests all be their own workflow that is seems difficult to enforce this pr/pattern thing any other way? Absent that, seems like a matrix would work nicely but not sure how you would do this via matrix. Seems to make sense, just trying to make sure I understand why.

Not sure I understand the question. Each package gets its own workflow. Which workflows run depends on the structure of the package. The flutter_vm workflow is chosen for any package which depends on Flutter.

My bad for confusing phrasing (I also saw this bit/left this comment before I saw the autogeneration code). My point was that the similar workflow logic is running/repeated across these categories. You could run them via a matrix to save repitition like the integration tests but I'm not sure how you would stop one category from running on pushes to certain files with that pattern, so I thought that was why you did it this way. Knowing they are autogenerated would also be another reason.

Yeah, since paths affect the workflow as a whole and there isn't a way to control this at any lower level from what I've seen. Since it's autogenerated, there's no real maintenance risk to 1 workflow vs. 50. It's also why all the workflow logic is centralized in flutter_vm, so all these files do is control the paths to run on, essentially.

We also have more information about the packages in aft than we do once the workflow is running which is another reason we generate them.

ragingsquirrel3 · 2022-11-11T23:43:28Z

.github/workflows/dart_native.yaml

+        os:
+          # macos-latest currently points to macos-11
+          # https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idruns-on
+          - macos-12


callout (no change needed inthis PR): This is going to consume lots of macos compute across its consumers, so just might be something to keep an eye on if we hit a limit at some point. Also dk if people run workflows on their fork like I do where you can only run 5 mac jobs at once.

Good point. My thinking with these is that they will only run on pushes (not pull requests) so they can run asynchronously without blocking any real work. Open to further discussion, though (maybe run them on a schedule instead?)

Sure, can discuss in other forum. For now I think this PR has appropriate change bc good to test these on pushes, could discuss optimizations later. At the root I think a lot of compute consumption is just running aft bootstrap like we discussed for another change, so cutting down at that time would help a lot. The next thing to consider would be things like limiting matrix on pushes/schedules, etc... IMO.

ragingsquirrel3 · 2022-11-11T23:48:53Z

packages/aft/lib/src/commands/generate/generate_workflows_command.dart

+      final isDartPackage = package.flavor == PackageFlavor.dart;
+      final workflowContents = StringBuffer(
+        '''
+name: ${package.name}


nit/question: Should this include a comment that they are autogenerated in the output? I had no idea until I got here.

Good point. Updated 👍

ragingsquirrel3 · 2022-11-11T23:51:14Z

packages/auth/amplify_auth_cognito_test/test/flows/srp/srp_password_verifier_worker_test.dart

@@ -12,6 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

+// TODO(dnys1): Investigate DDC failures
+@Tags(['no-ddc'])


nit: Would we ever consider keeping tags in a centralized place as consts/enums? Like maybe in amplify_test? That way they're easier to keep track of/not repeat, etc...

This is definitely temporary. At some point, if we have more permanent tags used throughout, that could definitely make sense.

ragingsquirrel3 · 2022-11-11T23:52:50Z

packages/common/amplify_db_common/pubspec.yaml

@@ -11,7 +11,7 @@ environment:

 dependencies:
  amplify_db_common_dart: ^0.1.0
-  drift: ^2.2.0
+  drift: ^2.0.0


why is this changed here/other places?

The changes are from this commit be12ace

ragingsquirrel3 · 2022-11-12T00:01:15Z

.github/workflows/amplify_core.yaml

+      - next
+  pull_request:
+    paths:
+      - 'packages/amplify_core/**/*.dart'


Fore these patterns, I dk if would make more sense to specify what should NOT run the tests, like !**/*README.md than trying to state what SHOULD and leave this open to errors of omission, but I do not feel strongly one way or the other.

My feeling is that CI checks in PRs should do the absolute minimum amount of work to verify the change. This is best accomplished through an "opt-in" rule vs "opt-out". On push, CI checks run for any change and this is a better mechanism for catching things which weren't immediately obvious.

Adds base workflows for Dart & Flutter packages

Adds a new command for generating GitHub actions workflows

Updates goldens for new CI pipeline

Should be `master` instead of `main`

Generates GitHub Actions workflows for all packages in the repo

Removes mono_repo from the repository

Worker tests fail in DDC right now. Since they are never compiled in DDC mode when published, we don't need to worry about them for now.

Enables web tests in the `aws_signature_v4` package

`amplify_db_common` dependency constraints were set too high for minimum Dart target of 2.17.0

This was removed in `pkg:xml` where it happened automatically (renggli/dart-xml@366149f)

Allows packages to declare a `setup.sh` script to run before running tests to allow for pluggable package-specific setup work.

dnys1 requested a review from a team as a code owner November 8, 2022 23:41

dnys1 force-pushed the feat/new-ci branch 5 times, most recently from 7b7d251 to a59f95f Compare November 9, 2022 00:38

Jordan-Nelson reviewed Nov 9, 2022

View reviewed changes

dnys1 force-pushed the feat/new-ci branch 3 times, most recently from 2547004 to 38eef64 Compare November 10, 2022 18:59

ragingsquirrel3 reviewed Nov 11, 2022

View reviewed changes

ragingsquirrel3 approved these changes Nov 11, 2022

View reviewed changes

ragingsquirrel3 reviewed Nov 12, 2022

View reviewed changes

dnys1 force-pushed the feat/new-ci branch from 08db8b8 to 2255784 Compare November 12, 2022 00:27

dnys1 enabled auto-merge (rebase) November 16, 2022 17:04

Dillon Nys added 14 commits November 16, 2022 13:40

chore(aft): Remove pubspec_overrides.yaml

5c05082

chore(repo): Add base workflows

9b9713f

Adds base workflows for Dart & Flutter packages

feat(aft): Add generate workflows command

70d7af4

Adds a new command for generating GitHub actions workflows

ci(smithy): Update goldens for new CI

a9b1a4b

Updates goldens for new CI pipeline

fix(aft): Base ref for aws-models repo

a0698cb

Should be `master` instead of `main`

ci(repo): Generate new CI workflows

95a6ce6

Generates GitHub Actions workflows for all packages in the repo

ci(repo): Remove mono_repo

93cb854

Removes mono_repo from the repository

test(auth): Add no-ddc tag for worker tests

ada153f

Worker tests fail in DDC right now. Since they are never compiled in DDC mode when published, we don't need to worry about them for now.

ci(sigv4): Enable web tests

a16f43e

Enables web tests in the `aws_signature_v4` package

chore(db_common): Fix dependency constraints

3e49877

`amplify_db_common` dependency constraints were set too high for minimum Dart target of 2.17.0

chore(repo): Update dependencies

ede60ec

ci: Fix test check

a2e63e1

chore(smithy): Fix equality check for XmlName

027c8e3

This was removed in `pkg:xml` where it happened automatically (renggli/dart-xml@366149f)

chore(repo): Add new packages to CI

fadb95a

dnys1 force-pushed the feat/new-ci branch from 2255784 to f7aab97 Compare November 16, 2022 20:53

ci: Add setup step

542e6ce

Allows packages to declare a `setup.sh` script to run before running tests to allow for pluggable package-specific setup work.

dnys1 force-pushed the feat/new-ci branch 2 times, most recently from 02bf809 to 542e6ce Compare November 16, 2022 21:02

chore(secure_storage): Fix analysis issue

b99c495

Jordan-Nelson approved these changes Nov 16, 2022

View reviewed changes

dnys1 merged commit 7c9a253 into next Nov 16, 2022

dnys1 deleted the feat/new-ci branch November 16, 2022 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(repo): New CI pipeline #2331

ci(repo): New CI pipeline #2331

dnys1 commented Nov 8, 2022

Jordan-Nelson Nov 9, 2022

dnys1 Nov 9, 2022

Jordan-Nelson Nov 9, 2022

dnys1 Nov 9, 2022

Jordan-Nelson Nov 9, 2022

Jordan-Nelson Nov 9, 2022

dnys1 Nov 9, 2022

ragingsquirrel3 Nov 11, 2022

dnys1 Nov 12, 2022

ragingsquirrel3 left a comment

ragingsquirrel3 Nov 11, 2022

dnys1 Nov 12, 2022

ragingsquirrel3 Nov 11, 2022

dnys1 Nov 12, 2022

ragingsquirrel3 Nov 14, 2022

dnys1 Nov 14, 2022

ragingsquirrel3 Nov 11, 2022

dnys1 Nov 12, 2022

ragingsquirrel3 Nov 14, 2022

ragingsquirrel3 Nov 11, 2022

dnys1 Nov 12, 2022

ragingsquirrel3 Nov 11, 2022

dnys1 Nov 12, 2022

ragingsquirrel3 Nov 11, 2022

dnys1 Nov 12, 2022

ragingsquirrel3 Nov 12, 2022

dnys1 Nov 12, 2022

		- `generate`: Generates various repo items
		- `workflows`: Generates GitHub actions workflows for all packages in the repo

ci(repo): New CI pipeline #2331

ci(repo): New CI pipeline #2331

Conversation

dnys1 commented Nov 8, 2022

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

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

Choose a reason for hiding this comment

ragingsquirrel3 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment