-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Upgrade pact js core prebuilds ci #1098
Conversation
describe('#isPortAvailable', () => { | ||
(process.platform === 'linux' && process.arch === 'arm64' | ||
? describe.skip | ||
: describe)('#isPortAvailable', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yak shave - maybe due to node slim image, need to try with full fat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're best keeping an eye on this, networking related bugs are a pain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
110%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I had a look at the failing test. It's failing, because we're expecting it not to be able to find a free port on port 80
. But if it's running in a container (like Cirrus CI, I believe) then port 80
may well be allowed, and the test could pass.
I'll see if we can ensure it's "unavailable" by other means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes I see now that on windows it sets -1
, other platforms 80
.
yes cirrus-ci in our current config is using a Dockerfile (or image) as the CI environment
note cirrus-ci failing on PR's
|
Resolved by setting GIT_BRANCH and GIT_COMMIT via CIRRUS_* ENV vars. info page for cirrus https://cirrus-ci.org/guide/writing-tasks/#environment-variables best added into the pact-broker client, to capture these for Cirrus-CI users (of which we are for our arm64 builds :) ) |
barring
we should be good to go :) |
Going to get these changes merged in now and released, will get builds green.
We are in a much better place than before, with testing support for additional arches, and finally covering macos in ci again |
note: as this change also bumps the node engines version, it constitutes a major change, we could backport this change, without the node engines bump to v11.x, as this will release as v12.x just a thought, otherwise troubleshooting notes will state v10/v11 of pact-js require the python build chain, v12 onwards doesn't. (it might be wiser leaving it as is, as we have only tested the prebuilds from node 16 onwards, so the engine bump imo makes sense (as is also present in pact-js-core, so a pact-js user on node <16 would probably see an error from pact-js-core) |
Awesome! I agree with getting it in and bumping major version. |
Includes and tests pact-core 13.15.0 which includes
Requires for completeness
Some notes:-