Rename ci.yml to main.yml #167
Conversation
| - "**.rs" | ||
| - "Cargo.toml" | ||
| - "Cargo.lock" | ||
| on: [push, pull_request] |
There was a problem hiding this comment.
IMO it doesn't make sense to run it on push. The code is already on the branch (possibly in a broken state if someone pushed directly to master). That's why I set it to PR only and filter out for files that do affect the build. Changing the readme shouldn't trigger the whole run, it's a waste of resources. If you remove the filter, changes to README would still run the whole workflow.
So in my eyes the change doesn't make sense tbh. I would, however, set master as protected so even admins cannot push to it. Meaning we also have to go through PRs. This ensures, that the CI runs even against our code.
Edit: I added the protection rules for the master branch. This means that all PRs need to b reviewed by one of the admins. All CI checks except nightly/beta have to pass. As of now, the rules are not enforced on us, however. We can ignore it and still merge. There is enough red text though that indicates that this is a bad idea 😄. Once we have enough maintainers on board I'd like to enable it for admins as well. This doesn't really make sense for just two people that actively maintain a crate. But if we are like 5-ish, there should be always one person that can do a review :)
There was a problem hiding this comment.
@sirwindfield That's fair, I'm not sure we need the file filter though, since we could and possibly should be testing the code fragments in our README, and changes to CI should also run through CI.
Making the main branch protected sounds like a good change though!
There was a problem hiding this comment.
How do you want to test the readme fragments? Afaik, cargo doesn't test those anyway. There has been work on a proof-of-concept, but I wouldn't include that tbh: rust-lang/cargo#383.
This is apparently a semi-popular crate for this: https://github.com/budziq/rust-skeptic.
But again, I don't see the value here. IMO the examples directory should be enough and a readme is just nice to have.
In it's current state, the readme doesn't get tested, so I don't see the value of removing the paths. I'd probably change it from allowlist to denylist, i.e. use paths-ignore and list the files we do not want to trigger a run. Which would probably be *.md, the license files and the .gitignores file. Every other file should trigger the workflow, so that would make more sense. Not sure why I didn't think of this earlier...
There was a problem hiding this comment.
Ah, there's a library called rust-skeptic that lets you test readme samples, but it looks like it might not have been updated in a while.
I guess what I'm trying to get at is that it doesn't seem necessary to limit when CI runs based on an includelist glob, since you can come up with reasons to want to run it for just about any filetype, and new filetypes get introduced from time to time.
There was a problem hiding this comment.
@KodrAus ah, ok now I get it.
I guess what I'm trying to get at is that it doesn't seem necessary to limit when CI runs based on an includelist glob, since you can come up with reasons to want to run it for just about any filetype, and new filetypes get introduced from time to time.
Wouldn't an exclude list then make even more sense? that way, new files are added automatically and we have to manually put them into the filter.
|
I would suggest to add a |
|
On that note I named it CI as it is responsible for the continuous integration of PRs. I am working on a workflow that automatically publishes on crates and github when a new tag is pushed to the repo as well. So the names |
| @@ -1,11 +1,6 @@ | |||
| name: Continuous Integration | |||
|
|
|||
| on: | |||
| pull_request: | |||
| paths: | |||
There was a problem hiding this comment.
I just noticed that the filter should also include the workflow folder as changes to those should be run as well I think... 🤔
|
This specific changeset doesn't seem like something we'll want to pursue, so I'll go ahead and close it now, and not meddle with our CI until you've had a chance to explore the other features you had in mind @sirwindfield 🙂 |
Just tinkering with our GitHub Actions a bit.