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: update ci to add a cargo build and test on push/pr #37

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

jeremydk
Copy link
Contributor

@jeremydk jeremydk commented Aug 7, 2024

Currently, the build/release action takes place on push to main or tagged builds for release.

With more contributors, I thought it would be convenient to add a github action to run the cargo tests on push and PR.

Additionally, I realized that we would benefit a lot from dependency caching, so I have added that for both node and cargo targets.

@progre
Copy link
Owner

progre commented Aug 7, 2024

Thanks a lot!

Currently the main branch only does test. Instead of adding a new action, I thought it would be better to add push and pull_request to the existing action. What do you think?

@jeremydk
Copy link
Contributor Author

jeremydk commented Aug 7, 2024

We can roll it into a single, it's a trade-off between the duplication between the actions and the amount of conditional logic for branch logic for package builds that you want. Right now push is only being applied to commits on main. Adding it to PR would be an intermediate - just comes down to the validation you want on branches that are under development as well.

I personally tend to separate the build/test from build/release pipelines (since I tend to think of these as deliberate actions), but I think it's down to personal preference.

@progre
Copy link
Owner

progre commented Aug 7, 2024

Thanks for your valuable input.

Actually, the workflow is based on an edited version of the tauri-apps template and I don't have much control over it.
I don't intend to add more test jobs, so making test an independent workflow is a bit tempting.
However, I afraid that I will end up with more workflows that are difficult to control.

This time, instead of creating a new workflow, why not add an on condition to the existing workflow?

@jeremydk
Copy link
Contributor Author

jeremydk commented Aug 7, 2024

Updated to a single file -- It's gonna fire tests against all targets this way, which is a bit unnecessary but it doesn't matter. I'm not crazy about having all of the CI share write-permissions, but hopefully this is a bit more straightforward.

@progre
Copy link
Owner

progre commented Aug 7, 2024

    permissions:
      contents: write

Is it about this setting?
It certainly does not look good that anyone can get this permission through PR.

I should have accepted your suggestion.
Is it possible to have the proposal reverted to the previous proposal?

@jeremydk
Copy link
Contributor Author

jeremydk commented Aug 7, 2024

It would require a malicious update to the ci config - but the current permissions are what allow it to upload releases for you, so that's the trade off. I'll revert this last commit.

@progre
Copy link
Owner

progre commented Aug 7, 2024

Your comments was very helpful. Thank you!

@progre progre merged commit 5a4e8e1 into progre:main Aug 7, 2024
2 checks passed
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.

2 participants