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

Make CI run on changes to non-code files as well #2130

Merged
merged 1 commit into from Jan 5, 2022

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jan 3, 2022

The repository is configured such that specific checks are required to pass before merging; since CI doesn't run on PRs that don't change code, they can't be merged without administrator intervention.

Currently affecting #2126, and has affected many others in the past.

Also, while this definition is currently correct, we might in the future change something so that changes to other files (like markdown files) really should be run through CI.

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@madsmtm madsmtm added the S - maintenance Repaying technical debt label Jan 3, 2022
@maroider
Copy link
Member

maroider commented Jan 4, 2022

Ideally, we'd have CI skip any code-checking steps if no source code was touched, instead of running all the steps all of the time, but that's also a bit more complicated to set up.

Copy link
Contributor

@rukai rukai left a comment

Choose a reason for hiding this comment

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

LGTM
Running CI on all PRs is not that expensive and is far safer as there are a lot of ways to break things outside of changing code.

The repository is configured such that specific checks are required to pass before merging; since CI doesn't run on PRs that don't change code, they can't be merged without administrator intervention.

Also, while this definition is currently correct, we might in the future change something so that changes to other files (like markdown files) really should be run through CI.

And example of that could be:
```rust
#[cfg(doctest)]
#[doc = include_str!("../README.md")]
extern "C" {}
```
@madsmtm
Copy link
Member Author

madsmtm commented Jan 4, 2022

Ideally, we'd have CI skip any code-checking steps if no source code was touched, instead of running all the steps all of the time, but that's also a bit more complicated to set up.

Also not something neither you nor I can fix, I think it's part of the repo settings that only an admin/"owner" can access?

@maroider
Copy link
Member

maroider commented Jan 4, 2022

Also not something neither you nor I can fix, I think it's part of the repo settings that only an admin/"owner" can access?

It's something a more sophisticated CI config could solve.

@madsmtm
Copy link
Member Author

madsmtm commented Jan 4, 2022

Fair enough, I'll gladly admit I don't know enough about GitHub Actions.

Copy link
Member

@maroider maroider left a comment

Choose a reason for hiding this comment

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

This, while somewhat wasteful, is probably nicer than having to circumvent the "CI passes" check.

We should probably try to get a more granular CI config set up eventually.

@madsmtm madsmtm merged commit a033b25 into rust-windowing:master Jan 5, 2022
@madsmtm madsmtm deleted the fix-required-ci branch January 5, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - maintenance Repaying technical debt
Development

Successfully merging this pull request may close these issues.

None yet

3 participants