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

Make sure CI builds test latest versions of dependencies #570

Merged
merged 7 commits into from Apr 5, 2021

Conversation

cjbarth
Copy link
Collaborator

@cjbarth cjbarth commented Mar 19, 2021

This will prevent the CI builds from relying on package-lock.json. The only question that I have is about including package-lock.json in commits. I think we should keep it so that we can rebuilt exact artifacts at tags, but, OTOH, we might want to avoid using it because it would mask stuff that appears when doing development. I favored the rebuilding artifacts argument in this PR, but am open to discussion.

@cjbarth
Copy link
Collaborator Author

cjbarth commented Mar 19, 2021

@forty Would you like to have a look at this based on our discussion in #566 ?

@forty
Copy link
Contributor

forty commented Mar 22, 2021

@cjbarth That looks good to me 👍

@forty
Copy link
Contributor

forty commented Mar 22, 2021

Her, actually, 2 comments:

  • I noticed that you reverted the removal of the package-lock, but I think npm install will actually follow the package-lock if it's there
  • npm install runs the prepare script, which causes tsc to be invoked twice (since npm run test also invokes it explicitely) - no harm, except slower CI

@cjbarth
Copy link
Collaborator Author

cjbarth commented Mar 22, 2021

Basically, we have three choices:

  1. Use npm-shrinkwrap.json, which will lock packages for developers, CI, and consumers
  2. Use package-lock.json, which will lock packages for developers and CI, but not for consumers
  3. Use neither, which will mean that each installation, even for developers and CI builds will result in a different behavior

If we use 3, then we can't get reproducible builds from a tag. I'm not sure if that is what we're after.

@forty
Copy link
Contributor

forty commented Mar 22, 2021

One option would be to remove the file from Git and for the CI to publish the generated package-lock as an artifact, so we could always go back to a given build and reproduce it. We could also continue shipping it in the published package, even if it's ignored, but just as a kind of documentation.

Note that I don't know GH CI at all, so I don't really know what's possible/convenient.

@gugu
Copy link
Contributor

gugu commented Mar 22, 2021

I think it is better not to remove it. We can add one more test npm ci && npm update to test if the module runs on the latest version of dependencies

@forty
Copy link
Contributor

forty commented Mar 22, 2021

That works too indeed, but note that you would need something like npm update --depth 99 to update sub dependencies.

Another option is to do the extra test with rm package-lock.json && npm install

@markstos
Copy link
Contributor

I'm in favor of keeping a lock file for consistent and reproducible results everywhere.

I think I'm most aligned with the shrinkwrap option. I use Yarn and yarn.lock in my own projects and do commit and version the file.

@cjbarth
Copy link
Collaborator Author

cjbarth commented Mar 22, 2021

It appears that we need to do something. The lockfile being the way it is allows for the tests to run and pass. However, if I delete package-lock.json and delete node_modules and then npm install. The tests fail with the following:

TypeError: lib/passport-saml/index.js: Emit skipped
    at getOutput (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:755:17)
    at Object.compile (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:968:32)
    at Module.m._compile (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:1056:42)
    at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Object.require.extensions.(anonymous function) [as .js] (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:1059:12)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/home/cjbarth/code/passport-saml/test/test-signatures.spec.ts:1:1)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Module.m._compile (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:1056:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:1059:12)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.exports.requireOrImport (/home/cjbarth/code/passport-saml/node_modules/mocha/lib/esm-utils.js:42:12)
    at Object.exports.loadFilesAsync (/home/cjbarth/code/passport-saml/node_modules/mocha/lib/esm-utils.js:55:34)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:834:11)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)

@forty
Copy link
Contributor

forty commented Mar 23, 2021

@markstos My understanding is that yarn.lock works similarly to npm's package-lock.json and is ignored for non "top level" package (so the argument of making sure the tests passes without still holds)

I think shrinkwrap is more or less deprecated, basically since yarn was launched, as their main "innovation" at the time was precisely to ignore shrinkwraps files for libraries if I remember correctly.

@forty
Copy link
Contributor

forty commented Mar 23, 2021

@cjbarth It was just a tiny problem https://github.com/node-saml/passport-saml/pull/572/files

It works with recent packages after that

@markstos
Copy link
Contributor

@forty That sounds right.

I used shrinkwrap files years ago and there were many problems which is why we switched to Yarn.

@cjbarth
Copy link
Collaborator Author

cjbarth commented Mar 23, 2021

Ok, things look better now. @markstos @forty , did you want to review this?

@cjbarth cjbarth changed the title Remove dependency on package-lock.json for CI builds Make sure CI builds test latest versions of dependencies Mar 23, 2021
@forty
Copy link
Contributor

forty commented Mar 23, 2021

Looks good to me 👍

@cjbarth cjbarth added the semver-minor This PR requires a semver-minor version bump label Mar 27, 2021
@cjbarth cjbarth added this to the 3.0.0 milestone Apr 4, 2021
@cjbarth cjbarth merged commit 0798e4d into node-saml:master Apr 5, 2021
3 checks passed
@cjbarth cjbarth deleted the remove-package-lock-ci branch April 5, 2021 17:44
@cjbarth cjbarth mentioned this pull request May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor This PR requires a semver-minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants