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: Remove MongoDB runner #1719

Closed
wants to merge 6 commits into from

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Feb 1, 2023

Pull Request

Issue

The mongodb runner has a dependency that has to be downloaded over ssh and causes the CI to take ~17 minutes on Node 14

Closes #1730

Approach

  • Update CI dependencies
  • Improve node cache
  • Add mongodb github action
  • Saves 14 minutes on CI time

@parse-github-assistant
Copy link

parse-github-assistant bot commented Feb 1, 2023

Thanks for opening this pull request!

  • ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

@dplewis dplewis changed the title Remove mongo runner ci: Remove mongo runner Feb 1, 2023
@dplewis dplewis changed the title ci: Remove mongo runner ci: Remove MongoDB runner Feb 1, 2023
@dplewis dplewis requested a review from a team February 1, 2023 18:15
@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Base: 99.89% // Head: 99.89% // No change to project coverage 👍

Coverage data is based on head (fb6d913) compared to base (1c96205).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##            alpha    #1719   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files          61       61           
  Lines        5991     5991           
  Branches     1373     1373           
=======================================
  Hits         5985     5985           
  Misses          6        6           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2023

  • If we replace mongodb runner with a GH action won't it be more difficult for developers to develop and test a PR locally for various MongoDB version?
  • Could we just configure git to define the protocol that git uses to download that specific dependency?

@dplewis
Copy link
Member Author

dplewis commented Feb 1, 2023

If we replace mongodb runner with a GH action won't it be more difficult for developers to develop and test a PR locally for various MongoDB version?

Thats what the contribution guide is for, they can install mongodb-runner themselves, use MongoDB CLI or any method they choose for testing.

Could we just configure git to define the protocol that git uses to download that specific dependency?

Seems like extra work for a package that isn't a dev dependency.

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2023

MongoDB runner is only a dev dependency, so this issue doesn't have any effect on production deployments, right?

How come we don't see this issue with parse-server? Or does it occur there as well?

Maybe we could avoid adding a barrier for contributors and instead fix this temporary issue (we'll remove Node 14 support in 11 months) with a git config.

Configuring git shouldn't be a lot of work. We can instruct git to use a specific protocol for a specific dependency with a 1 liner. What is the problematic URL and what URL should the npm installer use instead?

@dplewis
Copy link
Member Author

dplewis commented Feb 1, 2023

MongoDB runner is only a dev dependency

It isn't, check package.json. You must be thinking about the server.

Should we add it as a dev dependency it would add 5 minutes to the CI?

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2023

Do you know why mongodb runner isn't a dev dependency? Does the SDK actually need it?

@dplewis
Copy link
Member Author

dplewis commented Feb 1, 2023

The SDK doesn't need it. I forgot why it isn't a dev dependency, it's always been like that. We have had a lot of issues with mongodb-runner over the years.

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2023

Then we can fix 2 issues at once here. Let's move it to dev and add a git config rule.

And down the road we can look for a replacement for the runner. I still think it would be good to keep it easier for contributors to test against different mongoDB versions. That's why we've added the package scripts in parse server. Maybe we could add them here too.

@dplewis
Copy link
Member Author

dplewis commented Feb 1, 2023

I think updating the Contributing Guide would solve all your concerns for the developers. I'm against adding unmaintained packages here.

Maybe we could add them here too.

I'm against that as we have flaky tests. Maybe once we figure out those I would consider it.

@dplewis
Copy link
Member Author

dplewis commented Feb 1, 2023

@mtrezza I actually tested moving as a dev dependency before this PR and npm ci took 7 minutes. I don't think developer will want to wait that long when testing locally.

https://github.com/dplewis/Parse-SDK-JS/actions/runs/4058503220/jobs/6986789849

Can we merge this?

@dplewis
Copy link
Member Author

dplewis commented Feb 2, 2023

@mtrezza Can you rerun the CI? I want to make sure the cache works

Screen Shot 2023-02-02 at 11 46 58 AM

@mtrezza mtrezza closed this Feb 2, 2023
@mtrezza mtrezza reopened this Feb 2, 2023
@dplewis
Copy link
Member Author

dplewis commented Feb 3, 2023

@mtrezza i think this is good to merge

@mtrezza
Copy link
Member

mtrezza commented Feb 3, 2023

Still not sure about removing mongodb runner for contributors to fix a temp issue with Node 14. I'll look into the git URL fix.

Also the dev dependency situation seems still unclear, why is the runner not a dev dependency if it's only needed in dev.

@dplewis
Copy link
Member Author

dplewis commented Feb 3, 2023

Still not sure about removing mongodb runner for contributors to fix a temp issue with Node 14. I'll look into the git URL fix.

I don’t see this being an issue for contributors. If it is an issue we can move this unmaintained package to dev dependency. There are many ways to install mongodb locally.

@dplewis dplewis requested a review from mtrezza February 3, 2023 16:17
@dplewis dplewis requested a review from dblythy February 3, 2023 22:04
@mtrezza
Copy link
Member

mtrezza commented Feb 4, 2023

What would be the steps required for a contributor to run a test with 3 different versions of MongoDB?

In Parse Server it's currently:

npm run test:mongodb:4.2.19
npm run test:mongodb:5.3.2
npm run test:mongodb:6.0.2

It is strange that mongodb runner works fine for Node 14 in the Parse Server CI, but not here.

@dplewis dplewis closed this Feb 4, 2023
@dplewis dplewis deleted the remove-mongo-runner branch February 4, 2023 02:23
@dplewis dplewis restored the remove-mongo-runner branch February 4, 2023 03:19
@dplewis dplewis reopened this Feb 4, 2023
@dplewis dplewis closed this Feb 4, 2023
@dplewis dplewis deleted the remove-mongo-runner branch February 4, 2023 03:50
@mtrezza
Copy link
Member

mtrezza commented Feb 4, 2023

I'v opened #1730 since this PR doesn't have a ref to an issue and has been closed.

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.

Node 14 CI takes long to finish
2 participants