Skip to content

ci: don't use unmaintained actions-rs actions#45

Merged
JonasKruckenberg merged 8 commits intodevfrom
ci/actions-rs
Jan 6, 2023
Merged

ci: don't use unmaintained actions-rs actions#45
JonasKruckenberg merged 8 commits intodevfrom
ci/actions-rs

Conversation

@FabianLars
Copy link
Copy Markdown
Member

@FabianLars FabianLars commented Jan 2, 2023

Low effort tbh, but i personally have no interest in maintaining a fork of some actions-rs actions.

For cargo-audit this discussion may be interesting: rustsec/rustsec#773 (comment)

For clippy/fmt i think annotations are overkill and i'd much rather have human readable compiler output than this JSON madness

Copy link
Copy Markdown
Member

@Beanow Beanow left a comment

Choose a reason for hiding this comment

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

Because these actions are used in many repos. What's your thought on trying to centrally maintain, or have some kind of leading example?

Comment on lines +32 to +39
- name: Download cargo-audit
run: |
set +f
curl -s https://api.github.com/repos/rustsec/rustsec/releases/latest | grep "browser_download_url.*cargo-audit-x86_64-unknown-linux-musl.*" | cut -d : -f 2,3 | tr -d \" | wget -qi -
tar -xvzf ./cargo-audit*.tgz
mv ./cargo-audit*/cargo-audit ./cargo-audit
chmod +x ./cargo-audit
set -f
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.

Seems brittle, I assume the reason is to download a prebuilt binary vs cargo install cargo-audit?

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.

Note, this is what one of the forks seems to do.
But with caching, to get similar speeds for most builds.

https://github.com/actions-rust-lang/audit/blob/0d6847edc7cf5cb0fd3aa8a3b6ee3085daf8b992/action.yml#L42-L46

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.

Also targeting /latest and by extension not having checksums or the like, yeah. That's waiting for flakes. 😅

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 apparently forgot that caching exists...

Also i didn't know that audit-check does nothing else than cargo install cargo-audit - i thought it does something more fancy - will do that too then, unless we switch to actions-rust-lang (see my other comment) or if we want to create our own fork - an action is probably good because of the issue it can create.

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.

Well the caching of it, so it doesn't cargo install when cache is available, is what I think the fancy part is 😆
As well as version pinning I suppose.

runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions-rs/audit-check@v1
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.

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.

rustsec was on my radar hence the linked issue, but their fork doesn't seem to fix the issues. it's not really more maintained than actions-rs (most prominently it still has using: node12 in action.yml

i didn't know of actions-rust-lang yet but it's a complete rewrite (in python) so someone should probably review it before we use it?

@FabianLars
Copy link
Copy Markdown
Member Author

Because these actions are used in many repos. What's your thought on trying to centrally maintain, or have some kind of leading example?

For the audit one, maybe especially with tauri's focus on security. But maybe actions-rust-lang you mentioned is good enough.

For the cargo-* actions not at all, i always disliked them and maintaining something you don't agree with is probably a recipe for disaster - tbh i'd like to drop these actions in all our repos, i'd much rather see actual compiler output then being limited to the action's annotations (because the compiler outputs only json when the action is used which is not human readable in the logs)

@Beanow
Copy link
Copy Markdown
Member

Beanow commented Jan 6, 2023

For the cargo-* actions not at all, i always disliked them and maintaining something you don't agree with is probably a recipe for disaster - tbh i'd like to drop these actions in all our repos, i'd much rather see actual compiler output then being limited to the action's annotations (because the compiler outputs only json when the action is used which is not human readable in the logs)

Oh 100%. If I think of this with some https://teamtopologies.com/ structures in mind, moving them into a centrally maintained thing, would be mean the "common workflows" team are providing workflows as-a-Service. While multiple devs consume those workflows.

Both the experience of consuming them, and the maintenance, are worth looking at.
In this case, poor logging would be a 👎 from the consumer side.

@FabianLars FabianLars requested a review from a team as a code owner January 6, 2023 13:31
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.

3 participants