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

Upgrade pact js core prebuilds ci #1098

Merged
merged 17 commits into from
Jul 10, 2023
Merged

Conversation

YOU54F
Copy link
Member

@YOU54F YOU54F commented Jul 6, 2023

Includes and tests pact-core 13.15.0 which includes

Requires for completeness

Some notes:-

  1. some binary content issues as per Binary Content Matching is odd - cross platform/arch pact-js-core#447
  2. includes cirrus-ci to test arm64 on linux and macos
  3. updates examples
  • skips mocha-pact and jest-pact examples until they are updated to work with the latest pact-js
  • updates an xml dep as it didn't have binaries for arm64 on the v3/e2e project
  • currently skips plugin tests on windows as verifier is failing, trying to isolate issue in this branch https://github.com/pact-foundation/pact-js/actions/runs/5479635684
  • reinstates macos tests on github actions, resolved one set of tests by increasing mocha timeout (had to update to .mocharc in the process and mocha-opts file is deprecated), removed clean up which was for the old ruby processes.

describe('#isPortAvailable', () => {
(process.platform === 'linux' && process.arch === 'arm64'
? describe.skip
: describe)('#isPortAvailable', () => {
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

110%

Copy link
Member

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.

Copy link
Member Author

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

package.json Outdated Show resolved Hide resolved
examples/v3/e2e/test/helper.ts Outdated Show resolved Hide resolved
@YOU54F
Copy link
Member Author

YOU54F commented Jul 6, 2023

note cirrus-ci failing on PR's

PactBroker::Client::Error - Command `git rev-parse --abbrev-ref HEAD` didn't return anything that could be identified as the current branch.

@YOU54F
Copy link
Member Author

YOU54F commented Jul 7, 2023

note cirrus-ci failing on PR's

PactBroker::Client::Error - Command `git rev-parse --abbrev-ref HEAD` didn't return anything that could be identified as the current branch.

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 :) )

@YOU54F
Copy link
Member Author

YOU54F commented Jul 7, 2023

barring

  1. some doc updates to mention that the node-gyp build system is only required for versions of pact js v10 and v11. v12 will be batteries included,

  2. this yak shave https://github.com/pact-foundation/pact-js/pull/1098/files#r1254869004 which we hadn't identified before as we didn't test on linux arm64

we should be good to go :)

mefellows added a commit to pact-foundation/pact_broker-client that referenced this pull request Jul 8, 2023
@YOU54F
Copy link
Member Author

YOU54F commented Jul 10, 2023

Going to get these changes merged in now and released, will get builds green.

  • will update docs post release
  • will raise tickets to track the yak shaves
    • provider plugin test on windows ci
    • is port available check in Docker environments

We are in a much better place than before, with testing support for additional arches, and finally covering macos in ci again

@YOU54F YOU54F merged commit a8e53d9 into master Jul 10, 2023
25 checks passed
@YOU54F
Copy link
Member Author

YOU54F commented Jul 10, 2023

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)

@mefellows mefellows deleted the upgrade-pact-js-core-prebuilds-ci branch July 10, 2023 22:21
@mefellows
Copy link
Member

Awesome! I agree with getting it in and bumping major version.

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

Successfully merging this pull request may close these issues.

Value of engines is incorrect in package.json Avoid compiling native dependencies for common build targets
2 participants