Skip to content

chore(layers): moved layer-publisher into npm workspace #1292

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

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Feb 16, 2023

Description of your changes

This PR introduces changes to the files related to publishing public layers that are aimed at:

  • Moving the utility into the project's npm workspace
  • Reducing technical debt on this area

The first point has already been explained at length in the linked issue (#1226), but the TL;DR; is that by bringing this code into the workspace we can make sure all shared dependencies (which happen to be almost all of them) are version aligned. This is especially desirable given that this code is CDK-heavy and that library infamously doesn't play well with version mismatches.

In regards to the second point, the following high level changes have been made:

  • Renamed folder from layer-publisher to layers (same as Powertools for Python)
  • Removed layers from pre-commit / pre-push hooks, all tests are done on the PR. Maintainers can still run the tests locally as needed albeit manually.
  • Changed unit test structure to align more closely with the rest of the project. Also changed testing strategy moving away from snapshot testing and towards making assertions on the resources present in the generated stack.
  • Changed integration test structure to align more closely with the rest of the project. Also broken down the various test cases to allow maintainers to quickly narrow down the area of code in case of issues.
  • Simplified the layer publisher CDK stack by removing all logic related to spawning child processes (now done directly in the test runner)

How to verify this change

There are three areas that would allow us to definitively say the changes are correct, one of which we can't test until after merging this PR.

The first one is by looking at the checks under this PR. The tests running as a part of this PR include unit tests of the changes. The second one are integration tests, which create a layer similar to how it would be done once pushing to prod and then tests that the file bundled in it are compatible with an actual Lambda function, here's a successful passing run.

The third one would be to actually publish the layers to the production AWS environment. Doing that however would cause the layer version number to increment, which is not desirable. For this reason we have opted to defer this test to the next release date, since the two previous tests already make us confident that any potential issue will be limited to the workflows in charge of publishing the layer, rather than the layer itself.

Related issues, RFCs

Issue number: #1226

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dreamorosi dreamorosi self-assigned this Feb 16, 2023
@dreamorosi dreamorosi linked an issue Feb 16, 2023 that may be closed by this pull request
2 tasks
@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label Feb 16, 2023
@github-actions github-actions bot added the internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) label Feb 16, 2023
@pull-request-size pull-request-size bot added size/XL PRs between 500-999 LOC, often PRs that grown with feedback and removed size/L PRs between 100-499 LOC labels Feb 17, 2023
@dreamorosi dreamorosi marked this pull request as ready for review February 17, 2023 14:39
@dreamorosi dreamorosi requested a review from am29d February 17, 2023 18:23
Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

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

Looks great! Two small changes:

Need to fix the path to the new file here to bin/layers.ts

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/1e36e45800119810d536a76427f86c24f9a3df35/layers/cdk.json#L2

@dreamorosi dreamorosi requested a review from am29d February 20, 2023 13:49
@dreamorosi dreamorosi merged commit c672c40 into main Feb 20, 2023
@dreamorosi dreamorosi deleted the 1226-maintenance-make-layer-publisher-part-of-the-main-npm-workspace branch February 20, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) size/XL PRs between 500-999 LOC, often PRs that grown with feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: make layer-publisher part of the main npm workspace
2 participants