Skip to content

GitHub actions#7035

Merged
davimacedo merged 48 commits into
masterfrom
githubActions
Dec 3, 2020
Merged

GitHub actions#7035
davimacedo merged 48 commits into
masterfrom
githubActions

Conversation

@davimacedo

Copy link
Copy Markdown
Member

No description provided.

@Moumouls

Moumouls commented Dec 3, 2020

Copy link
Copy Markdown
Member

you're right. I will go to the 18.04

I saw that you were fooled by the version selector like me!

@davimacedo

Copy link
Copy Markdown
Member Author

Yes.

Tests for 4.4.2 failed. Maybe we should address this issue on a separate pr. I will downgrade to 4.0.21.

@davimacedo

Copy link
Copy Markdown
Member Author

Now we just need to pray for the flaky tests do not fail :)

@Moumouls

Moumouls commented Dec 3, 2020

Copy link
Copy Markdown
Member

@davimacedo mongo version is correct i think, npm run lint seems to cancel the workflow don't know why

@davimacedo

Copy link
Copy Markdown
Member Author

@davimacedo

Copy link
Copy Markdown
Member Author

I will revert back to ubuntu 16

@Moumouls

Moumouls commented Dec 3, 2020

Copy link
Copy Markdown
Member

I didn't know that a failure in the matrix could cause the other matrix to crash.

@Moumouls

Moumouls commented Dec 3, 2020

Copy link
Copy Markdown
Member

@davimacedo it's not a problem with ubuntu it's the Node Version, mongodb runner is not usable under Node14/15

@davimacedo

Copy link
Copy Markdown
Member Author

By default, they cancel the other jobs within the same matrix but there is an option that you change this behavior. I think it is better to actually cancel though, right?

@Moumouls

Moumouls commented Dec 3, 2020

Copy link
Copy Markdown
Member

pull request was sent but new package version not released yet: mongodb-js/runner#174

@davimacedo

Copy link
Copy Markdown
Member Author

Makes sense.

@davimacedo

Copy link
Copy Markdown
Member Author

In this case I will go back to 18.04 and remove the newest node. We put it back once mongodb supports it. ok?

@Moumouls

Moumouls commented Dec 3, 2020

Copy link
Copy Markdown
Member

@davimacedo

Copy link
Copy Markdown
Member Author

finally passed through mongodb-runner

@davimacedo

davimacedo commented Dec 3, 2020

Copy link
Copy Markdown
Member Author

I think we are good now. Maybe wait to see if @dplewis has any additional thoughts and merge?

@Moumouls

Moumouls commented Dec 3, 2020

Copy link
Copy Markdown
Member

Finally the CI

@Moumouls

Moumouls commented Dec 3, 2020

Copy link
Copy Markdown
Member

@davimacedo is it possible the reduce the codecov threshold ?

@davimacedo

Copy link
Copy Markdown
Member Author

It looks it is possible: https://docs.codecov.io/docs/commit-status

@dplewis dplewis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@davimacedo davimacedo merged commit 54a61b7 into master Dec 3, 2020
@davimacedo davimacedo deleted the githubActions branch December 3, 2020 16:15
@mtrezza

mtrezza commented Feb 3, 2021

Copy link
Copy Markdown
Member

@dplewis I took a quick look at this because you mentioned it; looking into the CI logs I see some misconfigs like different MongoDB versions in pretest vs. test, also MongoDB >=4.2 is not compatible with MMAPv1 storage engine, mongo-runner will simply fail to start. Anyway, tests pass now for all recent MongoDB versions in #7161.

Comment thread .github/workflows/ci.yml
branches:
- '**'
env:
COVERAGE_OPTION: ./node_modules/.bin/nyc

@mtrezza mtrezza Apr 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@davimacedo just wondering, what is COVERAGE_OPTION used for? I haven't found any references.

Edit: Found a ref, I think that can be removed, maybe from travis ci?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I believe it makes this to work.

@mtrezza mtrezza Apr 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How so?

I believe it was part of a script in package.json, that has been removed:

cross-env NODE_ENV=test TESTING=1 ./node_modules/.bin/babel-node $COVERAGE_OPTION ./node_modules/jasmine/bin/jasmine.js

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You might be right. Let's try to remove it and see what happens.

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.

4 participants