search

cloudevents / sdk-go

Go SDK for CloudEvents
https://cloudevents.github.io/sdk-go/
Apache License 2.0
836 stars 221 forks source link

Make a few comments a bit clearer #978

Closed duglin closed 1 year ago

duglin commented 1 year ago

Minor clean-up following https://github.com/cloudevents/sdk-go/pull/977

embano1 commented 1 year ago

Btw: with your fixes and reducing flaky integration tests, should we make them required to pass in CI before merging?

duglin commented 1 year ago

Btw: with your fixes and reducing flaky integration tests, should we make them required to pass in CI before merging?

I don't have a strong opinion on that since I assume we won't merge any PRs that have failing checks regardless of whether they're one of the 'required' ones or not.

embano1 commented 1 year ago

Reason I ask is that I can approve a change and use auto-merge once CI passes :) Enabling now.

duglin commented 1 year ago

I was wondering if that was the reason... I have to then ask... why not require all checks to be required? Would we ever want to merge stuff that had a failing check? If so, I'm not sure why we have a check we don't really care about.

embano1 commented 1 year ago

I was wondering if that was the reason... I have to then ask... why not require all checks to be required? Would we ever want to merge stuff that had a failing check? If so, I'm not sure why we have a check we don't really care about.

Most of the checks run super fast, i.e., before a maintainer does a review or when changes are pushed afterwards. But you raise a fair point. Right now, Github seems to have issues though: can't add the integration tests to required checks, so 🤷