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: change from circle to github actions #144

Merged
merged 17 commits into from
Mar 15, 2020

Conversation

tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Feb 5, 2020

added github actions in place of circleci

A repo admin will need to provide codecov with the token via the repo settings

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGES.md

@tac0turtle
Copy link
Contributor Author

odd github actions are not firing

@liamsi
Copy link
Member

liamsi commented Mar 5, 2020

odd github actions are not firing

They are now 👍 The error you are seeing is fixed here: #166

@tac0turtle
Copy link
Contributor Author

there are some todo's that can be done prioir to the merge or after:

  • remove circle
  • add codecov token

@tac0turtle tac0turtle marked this pull request as ready for review March 5, 2020 14:53
@tac0turtle
Copy link
Contributor Author

why cargo-audit is not firing:

It is recommended to add the paths: section into the workflow file, as it would effectively speed up the CI pipeline, since the audit process will not be performed if no dependencies were changed.

@liamsi
Copy link
Member

liamsi commented Mar 5, 2020

why cargo-audit is not firing:

It is recommended to add the paths: section into the workflow file, as it would effectively speed up >the CI pipeline, since the audit process will not be performed if no dependencies were changed.

Can we do sth to fix this here? Not sure I understand this correctly. Because the path comment sounds like an optimization only (and not like an explanation why audit isn't triggered). But maybe I'm missing sth here.

@tarcieri
Copy link
Contributor

tarcieri commented Mar 5, 2020

Here's my recommendation for how to run cargo audit:

https://github.com/iqlusioninc/tmkms/blob/develop/.github/workflows/security_audit.yml

It's set to run on either a change to Cargo.toml/Cargo.lock, or to run once daily.

That recipe also caches the installed cargo-audit binary to ensure subsequent runs are fast because they don't have to compile it each time (at least in the event there was a cache hit). With that, you could potentially run it every time and make it blocking if you like.

@liamsi liamsi self-requested a review March 6, 2020 15:29
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Taking back the approval as @greg-szabo wants to have a look before we merge this.

@brapse
Copy link
Contributor

brapse commented Mar 9, 2020

@greg-szabo will look to merge this towards the end of the week, would be great to have your feedback before then 🙏

romac added a commit to informalsystems/hermes that referenced this pull request Mar 9, 2020
* Run cargo-audit daily and when dependencies have changed

See informalsystems/tendermint-rs#144 (comment)

* Change actions/checkout back to v2
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Left a few minor comments. This LGTM.

.github/workflows/audit.yml Show resolved Hide resolved
.github/workflows/audit.yml Show resolved Hide resolved
.github/actions-rs/grcov.yml Outdated Show resolved Hide resolved
llvm: true
output-type: lcov
output-file: ./lcov.info
prefix-dir: /home/user/build/
Copy link
Member

Choose a reason for hiding this comment

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

Is that a default path from github actions?

@@ -0,0 +1,13 @@
branch: true
Copy link
Member

@liamsi liamsi Mar 10, 2020

Choose a reason for hiding this comment

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

As a general note on code coverage, it would be good to make sure we didn't substantially alter the config and will report very different cov numbers after merging. I doesn't seem so but I didn't look into the type-script code for this action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed this (too late!)

I'd love to give another shout out to Tarpaulin. It's pretty much the go-to coverage tool for the Rust community, IMO:

https://github.com/actions-rs/tarpaulin

I've personally experienced a lot of false negatives trying grcov in the past. I don't know if they've gotten any better, but if you see things you think should be covered under grcov which it reports aren't... maybe check out tarpaulin.

Here's one of my example configs, FWIW:

https://github.com/RustCrypto/AEADs/blob/master/.github/workflows/workspace.yml#L41

@liamsi
Copy link
Member

liamsi commented Mar 11, 2020

No objections from my side. I think this could be merged as is. Would be good if #144 (comment) were addressed sooner than later, too.

@greg-szabo
Copy link
Member

This looks like a nice start for using GitHub Actions. It achieves the goal of moving away from CircleCI and implements the basic set of tests that we had in CircleCI with less scripting and more modular methods.
I can see improvements in it that I would like to work on in a next PR, like using caching for Rust in general and I'm not really sure why auditing is cached (and then not used). But those are details we can work on after this gets merged.

Let's merge this as soon as possible, so I can put additional work on top of it.

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

👍

@greg-szabo greg-szabo merged commit 00f4390 into informalsystems:master Mar 15, 2020
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…ems#22)

* Run cargo-audit daily and when dependencies have changed

See informalsystems/tendermint-rs#144 (comment)

* Change actions/checkout back to v2
hu55a1n1 pushed a commit to cosmos/ibc-rs that referenced this pull request Sep 29, 2022
* Run cargo-audit daily and when dependencies have changed

See informalsystems/tendermint-rs#144 (comment)

* Change actions/checkout back to v2
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.

CI: switch to Github Actions?
5 participants