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

Document casing of C implementations of SQL functions #230

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

JelteF
Copy link
Collaborator

@JelteF JelteF commented Sep 30, 2024

This is to make it easy to grep for these functions.

This is to make it easy to grep for these functions.
@JelteF JelteF added the documentation Improvements or additions to documentation label Sep 30, 2024
@JelteF
Copy link
Collaborator Author

JelteF commented Sep 30, 2024

Ugh... Why is skipping builds so hard if you also want to have branch-protections... : https://www.pantsbuild.org/blog/2022/10/10/skipping-github-actions-jobs-without-breaking-branch-protection

@wuputah
Copy link
Collaborator

wuputah commented Sep 30, 2024

Mmmm, more CI hackery. We could turn off the path restrictions given the CI is quite quick now?

That said... while I'm certainly not opposed to the branch protection rules, if they're just causing problems, maybe we don't need them. Personally, I'm a fan of setting rules and trust people will do the right thing, rather than enforce the rules in software. In this case, that would mean no branch protection rules [for CI], but trust that committers will only merge if the tests pass. I don't think we've ever merged without tests passing, so we're not really solving a problem with the branch protection rules.

But... the auto-merge feature is nice.

@JelteF
Copy link
Collaborator Author

JelteF commented Sep 30, 2024

Mmmm, more CI hackery. We could turn off the path restrictions given the CI is quite quick now?

Yeah, I think that's probably the most pragmatic solution. In ~95% of the PRs we change some of these paths anyway. Really only doc-only PRs benefit from not running CI.

Personally, I'm a fan of setting rules and trust people will do the right thing, rather than enforce the rules in software.
In this case, that would mean no branch protection rules [for CI], but trust that committers will only merge if the tests pass. I don't think we've ever merged without tests passing, so we're not really solving a problem with the branch protection rules.

Agreed

But... the auto-merge feature is nice.

Yeah, this is the main reason I would like to keep the branch protection rules. If we could have auto-merge work without them, I'd be all for not having required checks in the branch protection.

@JelteF JelteF mentioned this pull request Sep 30, 2024
JelteF added a commit that referenced this pull request Sep 30, 2024
We were skipping CI when certain files were not touched. This is prone
to causing problems.

1. Because it's an include and not exclude list, we need to be careful
to add files to it. This caused some confusing in #98
2. Branch protection rules and this skipping work terribly together,
because the branch protection rules will not allow merging the PR unless
the job was actually run. We ran into this in #230. There are ways to
work around this, such as described in the following blogpost, but it's
quite a bit of hackery:
<https://www.pantsbuild.org/blog/2022/10/10/skipping-github-actions-jobs-without-breaking-branch-protection>

So the most pragmatic solution seems to be not to skip CI anymore. CI is
now pretty quick so the benefit of skipping it is already fairly small,
and the number of PRs where we can actually skip CI is also really
limited (basically only for docs-only PRs).
@JelteF JelteF enabled auto-merge (squash) September 30, 2024 18:05
@JelteF JelteF merged commit 3aca03a into main Sep 30, 2024
3 checks passed
@JelteF JelteF deleted the improve-contributing branch September 30, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants