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(build): Update contributing.md and fix npm ci #417

Merged
merged 8 commits into from
Jan 12, 2022
Merged

fix(build): Update contributing.md and fix npm ci #417

merged 8 commits into from
Jan 12, 2022

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Jan 5, 2022 •
edited
Loading

Description of your changes

fix(build): Update contributing.md and fix npm ci

Changes:

  • Update CONTRIBUTING.md to use npm ci
  • Fix CONTRIBUTING.md based on markdown lint recommended fixes
  • Setup project references

closes #415

How to verify this change

git clean -fdx; npm ci

Related issues, RFCs

#415

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
  • 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 in downstream module
  • The PR title follows the conventional commit semantics

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

Michael Brewer added 6 commits January 5, 2022 11:18
@michaelbrewer
fix(build): Update contributing.md and fix npm ci
844633f 
Changes:
- Update CONTRIBUTING.md to use
> [email protected] preinstall
> (cd packages/commons && npm ci); (cd packages/logger && npm ci); (cd packages/metrics && npm ci); (cd packages/tracing && npm ci);

> @aws-lambda-powertools/[email protected] prepare
> npm run build

> @aws-lambda-powertools/[email protected] build
> tsc

added 586 packages, and audited 587 packages in 4s

84 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

> @aws-lambda-powertools/[email protected] prepare
> npm run build

> @aws-lambda-powertools/[email protected] build
> tsc

added 488 packages, and audited 489 packages in 4s

72 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

> @aws-lambda-powertools/[email protected] prepare
> npm run build

> @aws-lambda-powertools/[email protected] build
> tsc

added 822 packages, and audited 852 packages in 10s

91 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

> @aws-lambda-powertools/[email protected] prepare
> npm run build

> @aws-lambda-powertools/[email protected] build
> tsc

added 836 packages, and audited 859 packages in 8s

72 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

added 1327 packages, removed 2761 packages, changed 10 packages, and audited 1383 packages in 40s

100 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
- Add a preinstall task to install all of the sub packages

closes aws-powertools#415
@michaelbrewer
fix(build): Update contributing.md and fix npm ci
06452c3 
Changes:
- Update CONTRIBUTING.md to use npm ci
- Add a preinstall task to install all of the sub packages

closes aws-powertools#415
@michaelbrewer
chore: leave as npm install for now
e3b7f19
@michaelbrewer
Merge branch 'main' into fix-dev-build
a6bc544
@michaelbrewer
fix(all): setup up project references
9f7c8f9
@michaelbrewer
docs: update to use npm ci
e5557b5
@michaelbrewer
Copy link
Contributor Author

@dreamorosi would you have time to look at this ?

dreamorosi
dreamorosi reviewed Jan 7, 2022
View reviewed changes
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in the response.

We have been testing internally the issue and some preliminary results appear to show that npm install works with npm v7.x while npm v8.x throws the same error described in #415. We haven't settled yet on which version to use (see discussion) nor settled on how the local dev setup is going to look like (see here and here).

Regarding the changes to the tsconfig.json file I'd prefer having @flochaz have a look as he has been working on them more than me.

At this stage the only changes I'd feel comfortable approving are L58-L60 + L94 of CONTRIBUTING.md.

I'll request also a review from the other members to hear their opinion.

@dreamorosi dreamorosi added the on-hold This item is on-hold and will be revisited in the future label Jan 7, 2022
@michaelbrewer
Copy link
Contributor Author

ok sure @dreamorosi i would have assume most people keep their npm version up to date now. But at least for now we should signal that npm ci will have less side effects

@michaelbrewer
Copy link
Contributor Author

@flochaz - is there something wrong with this PR?

flochaz
flochaz approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@flochaz flochaz left a comment

Choose a reason for hiding this comment

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

Nope, sorry for the late review. LGTM

@dreamorosi dreamorosi added documentation Improvements or additions to documentation and removed on-hold This item is on-hold and will be revisited in the future labels Jan 12, 2022
@dreamorosi dreamorosi added this to the production-ready-release milestone Jan 12, 2022
@dreamorosi dreamorosi merged commit 279ad98 into aws-powertools:main Jan 12, 2022
flochaz added a commit that referenced this pull request Jan 14, 2022
@flochaz
Revert "fix(build): Update contributing.md and fix npm ci (#417)"
5ea9458 
This reverts commit 279ad98.
flochaz added a commit that referenced this pull request Jan 14, 2022
@flochaz
fix(all): fix latest release broken by change of npm pack result on c…
2c3df93 
…ommon (#470)

* Revert "fix(build): Update contributing.md and fix npm ci (#417)"

This reverts commit 279ad98.

* revert commons version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamorosi dreamorosi dreamorosi approved these changes

@flochaz flochaz flochaz approved these changes

@saragerion saragerion Awaiting requested review from saragerion

@ijemmy ijemmy Awaiting requested review from ijemmy

Assignees

@michaelbrewer michaelbrewer

Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
production-ready-release
Development

Successfully merging this pull request may close these issues.

Docs: unable to clone & install for development
3 participants
@michaelbrewer @flochaz @dreamorosi