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

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

Merged
merged 8 commits into from
Sep 29, 2022

Conversation

mickmister
Copy link
Contributor

@mickmister mickmister commented Sep 15, 2022

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 #29

Ticket Link

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

@mickmister mickmister requested a review from hanzei September 15, 2022 03:56
@mickmister mickmister changed the title Upgrade to cimg/go and allow node version to be passed in Upgrade to cimg/go and allow .nvmrc to be used Sep 15, 2022
@mickmister
Copy link
Contributor Author

@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 #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
Copy link
Contributor Author

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

Copy link
Member

@JulienTant JulienTant left a comment

Choose a reason for hiding this comment

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

LGTM 💯

plugin-ci/orb.yml Show resolved Hide resolved
@hanzei hanzei mentioned this pull request Sep 18, 2022
plugin-ci/orb.yml Outdated Show resolved Hide resolved
plugin-ci/orb.yml Outdated Show resolved Hide resolved
plugin-ci/orb.yml Outdated Show resolved Hide resolved
plugin-ci/orb.yml Show resolved Hide resolved
@hanzei
Copy link
Contributor

hanzei commented Sep 18, 2022

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

plugin-ci/orb.yml Outdated Show resolved Hide resolved
@mickmister mickmister requested a review from hanzei September 19, 2022 03:41
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

🚀

@mickmister
Copy link
Contributor Author

@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
Copy link
Contributor

hanzei commented Sep 19, 2022

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
Copy link
Contributor Author

@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
Copy link
Contributor

hanzei commented Sep 19, 2022

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
Copy link
Contributor Author

@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
Copy link
Contributor

hanzei commented Sep 19, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

hanzei commented Sep 19, 2022

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.

plugin-ci/orb.yml Outdated Show resolved Hide resolved
@hanzei hanzei changed the title Upgrade to cimg/go and allow .nvmrc to be used [MM-40754] Upgrade to cimg/go and allow .nvmrc to be used Sep 19, 2022
@hanzei hanzei added the 3: Reviews Complete All reviewers have approved the pull request label Sep 21, 2022
Copy link

@javaguirre javaguirre left a comment

Choose a reason for hiding this comment

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

Just a small comment about the Python version of the deploy

plugin-ci/orb.yml Outdated Show resolved Hide resolved
plugin-ci/orb.yml Outdated Show resolved Hide resolved
@hanzei
Copy link
Contributor

hanzei commented Sep 29, 2022

@mickmister Are we good to merge this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants