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

Add a single commit check workflow #494

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

daviddavis
Copy link
Contributor

[noissue]

@daviddavis
Copy link
Contributor Author

Stolen from Katello: https://github.com/Katello/katello/blob/master/.github/workflows/pull_request.yml

I'm imagining that this will be a non-blocking test. In other words, it won't prevent merges. It will just help to warn contributors/PR reviewers that there's more than one commit.

@daviddavis daviddavis force-pushed the single-commit branch 3 times, most recently from 9e9623f to 808e52f Compare September 9, 2021 17:57
@bmbouter
Copy link
Member

bmbouter commented Sep 9, 2021

Awesome! Will this be enabled by default or do plugins need to opt-in? I'm +1 to enabling by default and opting out, but I'm ok w/ whatever you decide.

@daviddavis
Copy link
Contributor Author

Thanks I forgot to set a default. I'm imagining it'll be enabled by default too.

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

I kind of liked the discourse announcement part. :) Thanks for doing this!

@daviddavis
Copy link
Contributor Author

daviddavis commented Sep 9, 2021

I kind of liked the discourse announcement part. :) Thanks for doing this!

Ha yea, that was a WIP I didn't mean to commit yet. Eventually I want to have the release automation automatically post the announcement to discourse.

@daviddavis daviddavis force-pushed the single-commit branch 4 times, most recently from 61f3d23 to 9c8bd55 Compare September 9, 2021 18:21
Copy link
Member

@rochacbruno rochacbruno left a comment

Choose a reason for hiding this comment

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

Nice! I will set it to false on galaxy_ng on next ci update.

We allow multiple commits because we enforce Squash and Merge.

@daviddavis daviddavis merged commit 7edf776 into pulp:master Sep 9, 2021
@quba42
Copy link
Contributor

quba42 commented Sep 13, 2021

I think I will also opt out pulp_deb. The PR volume is not so high that I feel the need to enforce single commits via automation.

@mdellweg
Copy link
Member

Can this check be used to set/delete a "multiple commit" label on the PR, instead of presenting a failed test?

@daviddavis
Copy link
Contributor Author

I think we could if we used a pull_request_target workflow. I personally like having a failing job as it's more obvious and shows near the merge button. But if you think we ought to use a label instead, I'd say open an issue, we can discuss more and I can look into it.

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.

5 participants