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

lookup: add crc32-stream #430

Merged
merged 1 commit into from
Sep 20, 2017
Merged

lookup: add crc32-stream #430

merged 1 commit into from
Sep 20, 2017

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Jun 2, 2017

Fixes: #429

Checklist

citgm crc32-stream using this lookup passes on my machine.

@gibfahn
Copy link
Member Author

gibfahn commented Jun 2, 2017

@ctalkington (re/ #429 (comment)), I've put you down as the maintainer of this module, so we'll ping you if we have any issues with it.

Basically citgm does (not real code):

VERSION=$(npm view crc32-stream | grep latest) # to get the version, e.g. 2.0.0
curl https://github.com/archiverjs/node-crc32-stream/archive/$VERSION.tar.gz | npm install
npm test

What usually breaks us is people changing their Github tags from 2.0.0 to v2.0.0 (which requires us to update the lookup table), removing the tests from their npm packages, or otherwise breaking that workflow. Other than that you shouldn't have to do anything.

Also we run all the module tests in parallel, so using 0 instead of hardcoded network ports is helpful.

@watilde
Copy link

watilde commented Jun 2, 2017

Nice! How about to adding node-archiver instead of crc32-stream btw...?

@gibfahn
Copy link
Member Author

gibfahn commented Jun 2, 2017

Nice! How about to adding node-archiver instead of crc32-stream btw...?

I have no real opinion, @ctalkington what do you think? Maybe we should add both. crc32-stream is a pretty tiny module, so it won't slow us down much, and the tests do fail with the latest master, so it's definitely catching something.

@gdams
Copy link
Member

gdams commented Jun 2, 2017

this module is able to run on FIPS

@ctalkington
Copy link

i say start with crc32-stream archiver mostly relies on other modules and i plan to make plugins register themselves in the future so if anything id test zip-stream or such down the road.

watilde

This comment was marked as off-topic.

targos

This comment was marked as off-topic.

@gibfahn
Copy link
Member Author

gibfahn commented Aug 10, 2017

CI 2: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/4/console

Just to make sure it's all still working.

@targos
Copy link
Member

targos commented Sep 19, 2017

@targos
Copy link
Member

targos commented Sep 20, 2017

Landed in feff5f5

@targos targos closed this Sep 20, 2017
@gibfahn gibfahn merged commit b4bf1e4 into nodejs:master Sep 20, 2017
@gibfahn gibfahn deleted the add-crc32 branch September 20, 2017 12:18
@gibfahn
Copy link
Member Author

gibfahn commented Sep 20, 2017

CI was green, landing.

@targos
Copy link
Member

targos commented Sep 20, 2017

@gibfahn I already landed it this morning

@gibfahn
Copy link
Member Author

gibfahn commented Sep 20, 2017

@targos amazing, I'd have thought Github would have caught that (I merged with the Github UI).

I'll push it off master again.

EDIT: Removed commit.

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

Successfully merging this pull request may close these issues.

5 participants