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.

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

chore: Add lockfile to integration-test project #570

Merged
merged 5 commits into from May 19, 2021
Merged

Conversation

akash1810
Copy link
Member

@akash1810 akash1810 commented May 19, 2021

What does this change?

We previously had issues with lockfiles in CI for the integration-test project. This caused a few issues and led to the changes in #569.

This change reverts those commits and adds a lockfile (might be easiest to review commits in order). That is, I'm not sure what the previous issues were, but they don't seem to exist anymore and we can use lockfiles in CI and gain deterministic builds.

Does this change require changes to existing projects or CDK CLI?

No.

Does this change require changes to the library documentation?

No.

How to test

Observe CI.

How can we measure success?

  • The addition of a lockfile and installation of dependencies using npm ci results in a deterministic build in CI
  • The re-addition of linting keeps the integration-test project in line with the main project

Have we considered potential risks?

n/a

I think this is failing because we do not observe a lockfile in the [integration-test project](https://github.com/guardian/cdk/blob/9b9a91bc03f507ba4d688024b4fc8059a617f9d0/integration-test/script/ci#L5-L8).
As `eslint-plugin-import`'s version includes `^`, we cannot guarantee what version gets used in CI.

We hadn't seen it locally as we don't do a fresh install. If you delete integration-test/node_modules and lint the integration-test project, the error can be seen.

Options:
  - Use pinned versions in the integration-test project (I'm not sure if dependabot works with pinned dependency versions)
  - Do not lint the integration-test project
  - Use a lockfile and install @guardian/cdk into the integration-test project with `--no-package-lock` (not sure if this is possible as @guardian/cdk will remain in `package.json`)

See:
  - import-js/eslint-plugin-import#2021
  - https://docs.npmjs.com/cli/v6/commands/npm-install#:~:text=--no-package-lock
@akash1810 akash1810 force-pushed the aa-lockfile branch 2 times, most recently from 33ca0fe to 6f119e7 Compare May 19, 2021 12:55
@akash1810 akash1810 changed the title chore: remind myself of lockfile issue chore: Add lockfile to integration-test project May 19, 2021
We previously had issues with lockfiles in CI for the `integration-test` project.
This caused a few issues and led to the changes in #569.

This change reverts those commits and adds a lockfile.
That is, I'm not sure what the previous issues were, but they don't seem to
exist anymore and we can use lockfiles in CI and gain deterministic builds.
@akash1810 akash1810 changed the title chore: Add lockfile to integration-test project chore: Add lockfile to integration-test project May 19, 2021
@akash1810 akash1810 marked this pull request as ready for review May 19, 2021 12:59
@akash1810 akash1810 requested a review from a team May 19, 2021 12:59
Copy link
Contributor

@jacobwinch jacobwinch left a comment

Choose a reason for hiding this comment

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

Great, glad to see the previous issues have mysteriously resolved themselves! 👍

@akash1810 akash1810 merged commit 476c0b2 into main May 19, 2021
@akash1810 akash1810 deleted the aa-lockfile branch May 19, 2021 13:14
@github-actions
Copy link
Contributor

🎉 This PR is included in version 15.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants