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

CI: test on Node 16, switch to newer cimg images #5137

Merged
merged 1 commit into from
May 3, 2021

Conversation

glasser
Copy link
Member

@glasser glasser commented Apr 28, 2021

Node 16.0.0 is now released; we should run CI against it.

The older circleci/node Docker images we use in the Apollo OSS orb do not yet
have 16.0.0. It seems reasonable to switch over to the "non legacy" cimg/node
Docker images anyway. Those images don't have "major version" tags like 16, just
tags like 16.0.0 and 16.0. So let's switch over to specifying images directly
in config.yml rather than via our orb, and let Renovate update it. See
https://github.com/renovatebot/renovate/blob/92aab2b30f5c0d811056a1574354e97057c42732/lib/manager/circleci/extract.ts#L49
for what Renovate is looking for.

This PR does not convert to using npm v7 (and its new package-lock.json format);
we should do that soon, perhaps on release-3.x.

@glasser glasser requested a review from abernix April 28, 2021 22:36
@glasser
Copy link
Member Author

glasser commented Apr 28, 2021

@abernix Curious about your thought on the approach here.

Note that the hapi tests fail. I think that is because only the very latest hapi release supports Node 16.

Not clear on whether a hapi upgrade can actually work in the AS2 context (see eg #3273); I see you have made it up to "@hapi/hapi": "19.2.0" on release-3.0 (this may require 20.1.2). So we may be able to say "Apollo Server 2 supports Node 16 except for apollo-server-hapi" or something.

@abernix
Copy link
Member

abernix commented Apr 29, 2021

@glasser I have already evaluated things like @hapi support on 2.x and opted to put it in the easier to reason about 3.x branch where it was just naturally compatible with the other upgrades. We've historically done some exceptional things for certain frameworks to allow the tests to pass (e.g., see any references to NODE_MAJOR_VERSION in our tests.

I wasn't thinking we'd introduce Node.js 16 to the 2.x range; I'd be pretty comfortable with us only introducing Node.js 16 testing (exclusively) on the release-3.0 branch as a motivator/incentivization for folks who want to live on the bleeding edge and use the alpha releases. Node.js 16 doesn't go LTS until October. You're picking up that branch pretty soon, so perhaps it's just a good time to start looking in that direction (only)?

@glasser
Copy link
Member Author

glasser commented Apr 29, 2021

Not trying on 2 sounds fair. I'm glad to see the only problem is in one package due to its main dep though.

Thoughts on how the cimg change is executed?

@abernix
Copy link
Member

abernix commented Apr 30, 2021

@glasser The executor change to docker.image seems reasonable, but note that I think it's fraught with one very real thing with regards to how Renovation would work. See my apollographql/federation#713 on federation which has a commit (and comment) explaining my thoughts there. See if that resonates with you.

(Though also, if that PR gets approved, I'm happy to apply the changes here myself.)

Node 16.0.0 is now released; we should run CI against it.

The older circleci/node Docker images we use in the Apollo OSS orb do not yet
have 16.0.0. It seems reasonable to switch over to the "non legacy" cimg/node
Docker images anyway. Those images don't have "major version" tags like 16, just
tags like 16.0.0 and 16.0.  So let's switch over to specifying images directly
in config.yml rather than via our orb, and let Renovate update it. See
https://github.com/renovatebot/renovate/blob/92aab2b30f5c0d811056a1574354e97057c42732/lib/manager/circleci/extract.ts#L49
for what Renovate is looking for.

This PR does not convert to using npm v7 (and its new package-lock.json format);
we should do that soon.
@glasser glasser changed the base branch from main to release-3.0 April 30, 2021 23:52
@glasser
Copy link
Member Author

glasser commented Apr 30, 2021

@abernix I've rebased this onto release-3.0 (which has the Hapi upgrade) and it passes!

Note that baseBranches on main in this repo does not contain release-3.0 so for better or for worse Renovate isn't running on release-3.0, so while this means Renovate won't help us keep these up to date on the branch, it also won't clobber them. That's OK for now I think.

(We could add release-3.0 to baseBranches but it'll still use the main config which seems bad. I think the best way to go here is rapidly get to the point where main is 3.x and 2.x is on its own branch and probably doesn't get renovated.)

@@ -38,7 +38,8 @@ jobs:
# at https://hub.docker.com/r/circleci/node/.

NodeJS 12:
executor: { name: oss/node, tag: '12' }
docker:
- image: cimg/node:12.22.1
steps:
- common_test_steps
# We will save the results of this one particular invocation to use in
Copy link
Member Author

Choose a reason for hiding this comment

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

Thoughts on if we should bump this to the 14 or 16 job?

Copy link
Member

Choose a reason for hiding this comment

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

I had written out a longer comment but somehow lost it. Let's wait and ticket this for follow-up in a few weeks? (Feel free to assign to me). Basically, this persist_to_workspace thing propagates things to the Tarballs and Publishing steps: https://github.com/apollographql/CircleCI-Orbs/blob/main/src/oss-ci-cd-tooling/orb.yml#L319-L346

Copy link
Member Author

Choose a reason for hiding this comment

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

still curious about this fwiw, but merging @abernix

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

LGTM. I think you said you expected a PR to renovate this, but also maybe experimented or understood better than I currently do that it would be constrained by something. We have had Renovate target multiple base Branches in the past. Happy to look at that.

@glasser
Copy link
Member Author

glasser commented May 3, 2021

@abernix My understanding is

  • you can configure renovate to target multiple base branches
  • but the configuration used is always the default-branch configuration
  • and currently we aren't targetting release-3.0 from that
  • and even if we were, the delta between release-3.0 and main's renovate configs is pretty high

My theory is that we do not have to wait for 3.0.0 to be released to either (a) merge release-3.0 into main and make a different branch for 2 or (b) make release-3.0 be the "default branch", perhaps. (Or that we'll get to 3.0.0 soon enough anyway.)

@glasser glasser merged commit e71c0b2 into release-3.0 May 3, 2021
@glasser glasser deleted the glasser/node-16 branch May 3, 2021 18:35
glasser added a commit that referenced this pull request May 3, 2021
Inspired by apollographql/federation#708

This also fixes engines declarations in the individual packages which
should have been done in #5137.
glasser added a commit that referenced this pull request May 3, 2021
Inspired by apollographql/federation#708 as well
as the Netlify change in
apollographql/federation#713

This also fixes engines declarations in the individual packages which
should have been done in #5137.
glasser added a commit that referenced this pull request May 3, 2021
Inspired by apollographql/federation#708 as well
as the Netlify change in
apollographql/federation#713

This also fixes engines declarations in the individual packages which
should have been done in #5137.
glasser added a commit that referenced this pull request May 3, 2021
Inspired by apollographql/federation#708 as well
as the Netlify change in
apollographql/federation#713

This also fixes engines declarations in the individual packages which
should have been done in #5137.

Also fix a weird problem related to `@apollo/protobufjs`. For some reason, Node 12 and 14 with npm 7 (but not Node 16!) was running into an issue where the fact that `@apollo/protobufjs` doesn't actually depend on a bunch of packages like `espree` that its CLI needs caused the `apollo-pbjs` calls to fail. I think when we forked, protobuf.js was midway through a refactor to make the CLI into a separate package, but we haven't set that up to work in our fork.  Adding all those big packages as dependencies of `@apollo/protobufjs` didn't seem great because that would make them non-dev dependencies of apollo-server themselves. Splitting the packages (and making the CLI package a devDep of apollo-reporting-protobuf) is the best long-term solution but for now adding the CLI deps as devDependencies of apollo-reporting-protobuf appears to work?
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants