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

Allow contributors to easily run CI locally with act #833

Merged
merged 1 commit into from Feb 18, 2022

Conversation

Shatnerz
Copy link
Contributor

Disable problematic jobs that involve Github Actions caching or cross whenever the environment is set to ACT. This allows running the CI pipeline locally and hopefully speeds up PR cycle times by reducing unexpected CI pipeline results.

Motivation

The CI pipeline does not run until maintainer approval. This allows an easy path for contributors to test out the CI pipeline locally and avoid unexpected results. I personally kept hitting issues here due to MSRV always throwing me off

Potential issues

  • act does not support Github Actions caching feature which is used in the fuzz workflow so I simply disabled it if act is detected.

  • the cross workflow is similarly disabled. I kept hitting sh: 1: cargo: not found (see: Build fails with sh: 1: cargo: not found cross-rs/cross#260). I tried a few different workaround but had no success.

I'm hoping this is acceptable as it still improves the local testing situation and covers the Tests workflow

@Shatnerz Shatnerz changed the title feat: Allow contributors to easily run CI locally WIP: feat: Allow contributors to easily run CI locally Feb 16, 2022
@Shatnerz Shatnerz changed the title WIP: feat: Allow contributors to easily run CI locally feat: Allow contributors to easily run CI locally Feb 16, 2022
@Shatnerz Shatnerz changed the title feat: Allow contributors to easily run CI locally Allow contributors to easily run CI locally with act Feb 16, 2022
@Shatnerz Shatnerz changed the title Allow contributors to easily run CI locally with act WIP: Allow contributors to easily run CI locally with act Feb 16, 2022
@Shatnerz
Copy link
Contributor Author

Shatnerz commented Feb 16, 2022

Sorry, moving to WIP. Looks like it works locally but has some minor issues on Github...

edit: and resolved by adding a .actrc

@Shatnerz Shatnerz force-pushed the feat/run-ci-locally branch 2 times, most recently from 805a6f7 to 8dea412 Compare February 17, 2022 00:13
@Shatnerz Shatnerz changed the title WIP: Allow contributors to easily run CI locally with act Allow contributors to easily run CI locally with act Feb 17, 2022
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

I feel your pain, Rust 1.29 is a massive PITA but I'm not convinced adding another tool is the best solution. Would it not be better if we could just configure CI to run once a new dev has had a single PR CI run approved i.e., manual approval was only needed for the first CI run?

@Shatnerz
Copy link
Contributor Author

Shatnerz commented Feb 17, 2022

I feel your pain, Rust 1.29 is a massive PITA but I'm not convinced adding another tool is the best solution. Would it not be better if we could just configure CI to run once a new dev has had a single PR CI run approved i.e., manual approval was only needed for the first CI run

@tcharding yeah I thought this might be a little contentious. Perhaps I should have labeled it as a PoC. I don't know if I would call this "adding another tool" as it is more along the lines of "more easily integrate with existing tools via zero impact changes".

I won't push hard. Ultimately the decision belongs to the maintainers, so I am fine if this is marked as an undesired changed and closed.

If the concern is continually supporting act anytime that we change our CI or perhaps if act introduces a breaking version, well that is a valid concern. In that case we could silently/unofficially support it. Remove the README section and expectations that act will work and then directly point it out as a potential option as needed when we see someone struggling CI failures in PRs.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 17, 2022

"more easily integrate with existing tools via zero impact changes" is something I happily support

I'm not sure if we can configure CI to auto-approve runs but I think we should if it's possible.

I didn't get the impression that README promises act would work but perhaps add this:

"We do not actively support act but will merge PRs fixing act issues."

Regarding the code, it looks fine, just please add newlines to ends of new files.

@apoelstra
Copy link
Member

Agreed. I have no intention of ever running this (you can already run ./contrib/test.sh locally but I agree the mess of env vars would be confusing to a newcomer) but it's noninvasive and easy to passively maintain.

concept ACK

apoelstra
apoelstra previously approved these changes Feb 17, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 8dea412

Maybe the README comment could be clearer that this is not really supported, but I'm fine with the current text.

@Shatnerz
Copy link
Contributor Author

Just pushed @Kixunil suggested change to explicit state that it isn't directly supported. Also added newlines at the ends of the new files.

apoelstra
apoelstra previously approved these changes Feb 17, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

Thanks.

ACK 4b8d342

contrib/act/event.json Outdated Show resolved Hide resolved
Disable problematic jobs that involve Github Actions caching or `cross`
whenever the environment is set to ACT. This allows running the CI
pipeline locally and hopefully speeds up PR cycle times by reducing
unexpected CI pipeline results.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 006193f

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 006193f

@tcharding
Copy link
Member

Its good with the boys, its good with me :) Thanks

@apoelstra apoelstra merged commit 1871c3a into rust-bitcoin:master Feb 18, 2022
@sanket1729 sanket1729 mentioned this pull request Feb 25, 2022
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.

None yet

4 participants