search

mattermost / circleci-orbs

Orbs used for building in MM repositories on circleci
3 stars 9 forks source link

[MM-40754] Upgrade to cimg/go and allow .nvmrc to be used #32

Closed mickmister closed 2 years ago

mickmister commented 2 years ago

Summary

This PR updates the plugin-ci orb to use Go 1.18, while also accepting a node version from the project's .nvmrc to use to install a specific version of node. If no file exists, it falls back to node v13.14

The PR is similar to https://github.com/mattermost/circleci-orbs/pull/29

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-40754

mickmister commented 2 years ago

@hanzei Please take a look at this when you get the chance. This both updates the Go version, and allows projects to specify their node version via a .nvmrc file. This will help plugin projects work with CI, no matter what node version they need to use.

I made this a separate PR from https://github.com/mattermost/circleci-orbs/pull/29 since there is more going on here (upgrading cimg and supporting a different node version), though both things need to happen at the same time here.

mickmister commented 2 years ago

@JulienTant Adding you as a reviewer since you dug into this a bit

hanzei commented 2 years ago

@mickmister Thanks for taking care of the migration to get CI green again

mickmister commented 2 years ago

@hanzei What's the process of getting this merged? I've tested this config by pasting it into the zoom plugin config and had CI run on that commit. Though, I haven't tested the most recent changes from your review.

hanzei commented 2 years ago

Could you please re run your test with the latest changes? Just to make sure they works as expected. Esp. the go module path change.

The logs contain the following line

npm WARN old lockfile npm WARN old lockfile The package-lock.json file was created with an old version of npm, npm WARN old lockfile so supplemental metadata must be fetched from the registry. npm WARN old lockfile npm WARN old lockfile This is a one-time fix-up, please be patient... npm WARN old lockfile

is this because we are using a newer npm version the usally? Should we be using an older version (by default) that doesn't do the migration?

mickmister commented 2 years ago

@hanzei Looks like I messed up on that run of CI. None of the jobs are set up to call the install-node command. That commit doesn't line up properly with what's in this PR. Let me test again

hanzei commented 2 years ago

Regarding the merge process: I'm confident that once you test the changes with zoom we can merge the PR. Once your other PRs care merged we need a new orb release. This is when it gets tricky. I don't have the power to create one and don't know who can that. Could you reach out to the devops team and request guidance from them? It would also be nice to fix the red CI on all PRs.

mickmister commented 2 years ago

@hanzei About the old lockfile instance in that CI run you mentioned, I was actually testing if .nvmrc would be used if supplied, so I purposely gave it a later version that would make the lockfile message show. I saw the message, and assumed that it read from .nvmrc. I should have instead explicitly checked what version nvm decided to use. I've now asserted that it correctly reads the .nvmrc file and uses its node version.

I copied this PR's config over to my test with the zoom plugin, and everything seems to be working correctly

hanzei commented 2 years ago

The logs look mostly fine. It seems like that in build (https://app.circleci.com/pipelines/github/mattermost/mattermost-plugin-zoom/2053/workflows/c6a41831-822b-4de1-8bb3-92df0e50deec/jobs/6407/parallel-runs/0/steps/0-103) the go dependencies get downloaded again. Do you know why this is the case?

mickmister commented 2 years ago

It seems like that in build (https://app.circleci.com/pipelines/github/mattermost/mattermost-plugin-zoom/2053/workflows/c6a41831-822b-4de1-8bb3-92df0e50deec/jobs/6407/parallel-runs/0/steps/0-103) the go dependencies get downloaded again. Do you know why this is the case?

@hanzei Taking a closer look, it is not downloading the exact same dependencies on the two occurrences. You can see it's downloading mattermost-server v6.7.0, then afterwards v6.0.2. This is because it's downloading the dependencies for build/go.mod, and then the dependencies for go.mod at the root of the repository.

This also brings up the point that we are not taking into account when the build/go.sum file changes in our caching. I'd like to handle that in a separate PR, as this is already pretty large scope. Also, it seems the caching isn't working correctly, if it's downloading all the dependencies. Is this your understanding as well?

hanzei commented 2 years ago

Your observation seems correct. Let's tackle the caching issues as part of https://mattermost.atlassian.net/browse/MM-45779. This PR already fixes a lot of things.

hanzei commented 2 years ago

@mickmister Are we good to merge this PR?