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

Implement GitHub Actions w/ codecov reports #681

Merged
merged 8 commits into from
Feb 12, 2021
Merged

Implement GitHub Actions w/ codecov reports #681

merged 8 commits into from
Feb 12, 2021

Conversation

kherock
Copy link
Collaborator

@kherock kherock commented Feb 12, 2021

Closes #627. This includes #600 and includes the test matrix I was working on before #637 was opened. It seems like there are some open file handle issues in the POST Object middleware that's causing a decent chunk of the test suite to fail on Windows.

@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@563b259). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #681   +/-   ##
=========================================
  Coverage          ?   88.82%           
=========================================
  Files             ?       20           
  Lines             ?     1351           
  Branches          ?        0           
=========================================
  Hits              ?     1200           
  Misses            ?      151           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 563b259...5843a2a. Read the comment docs.

@kherock
Copy link
Collaborator Author

kherock commented Feb 12, 2021

@jamhall any objection with migrating to main as the default branch? Just trying to avoid introducing a dependency on master as the default branch here.

@kherock
Copy link
Collaborator Author

kherock commented Feb 12, 2021

Also, this closes #636 since package-lock.json is needed to run the npm ci and npm audit commands.

Copy link
Contributor

@fishcharlie fishcharlie left a comment

Choose a reason for hiding this comment

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

Looks really great!!!

@@ -0,0 +1,75 @@
name: s3rver
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge deal. But I tend to suggest that naming is more specific for GitHub Actions workflows. If you ever want to add workflows to close stale issues, tag PRs, publish new versions, etc. those will tend to be different workflows. Which would make this name confusing since it's so general.

For Dynamoose we use CI. Not sure that makes the most amount of sense tho.

Suggested change
name: s3rver
name: CI

I also think it'd be valuable to have the name match the file name.


Super nit-picky, but I thought it was worth at least mentioning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CI makes enough sense to me, and thank you for giving me a new pet peeve for when I'm looking at other projects' actions 😄.

pull_request:
branches:
- master
- next
Copy link
Contributor

Choose a reason for hiding this comment

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

Any purpose for limiting branches here? Are there certain branches we don't want this to run on, and why?

Copy link
Contributor

Choose a reason for hiding this comment

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

The branch limitation avoids double building in case of pushing and immediately creates a pull request by the project developer. However, for this purpose it is enough to limit the branches for pushs.

@fishcharlie
Copy link
Contributor

Is this going to be the prioritized PR or is #677?

@fishcharlie
Copy link
Contributor

Looks like we also have some CI failures here. Looks like most of it is around Windows FS permissions.

@kherock
Copy link
Collaborator Author

kherock commented Feb 12, 2021

Is this going to be the prioritized PR or is #677?

There's still a decent amount of work ahead before it makes sense to start publishing Docker images for 4.x, so I'd like to get this in first.

@jamhall
Copy link
Owner

jamhall commented Feb 12, 2021

@kherock

@jamhall any objection with migrating to main as the default branch? Just trying to avoid introducing a dependency on master as the default branch here.

Nopes. No objection from me :)

@kherock
Copy link
Collaborator Author

kherock commented Feb 12, 2021

Great, could you rename the branch from the tab in repo settings? It'll automatically move all the PRs over once you do that.

Base automatically changed from master to main February 12, 2021 14:09
@jamhall
Copy link
Owner

jamhall commented Feb 12, 2021

Great, could you rename the branch from the tab in repo settings? It'll automatically move all the PRs over once you do that.

Hi @kherock - it's done

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.

Migrate to GitHub Actions
5 participants