search

cilium / design-cfps

Repo to store Cilium CFP design docs
Apache License 2.0
31 stars 26 forks source link

Add DCO checker on PRs #25

Closed joestringer closed 3 months ago

joestringer commented 8 months ago

It doesn't look like we have any bot to ensure the Developer's Certificate of Origin is signed on commits. It would be good to set this up.

hacktivist123 commented 8 months ago

I will take a look at implementing this 👍

hacktivist123 commented 7 months ago

This has been implemented @joestringer and @xmulligan

see this PR: https://github.com/cilium/design-cfps/pull/27/checks

joestringer commented 7 months ago

Awesome, thanks!

joestringer commented 5 months ago

Hi @hacktivist123 , the DCO doesn't seem to be showing up. For instance on this PR, there is a commit without signoff. I don't see any check at the bottom of the PR about this. https://github.com/cilium/design-cfps/pull/32

hacktivist123 commented 5 months ago

hey @joestringer i'm not sure why it doesn't show at the bottom however there is a check showing

Screenshot 2024-06-05 at 22 28 39
hacktivist123 commented 5 months ago

There is also a check on this PR

Screenshot 2024-06-05 at 22 32 02
joestringer commented 5 months ago

Hmm. I'm not familiar with this DCO app since we use a different mechanism in cilium/cilium. However, I don't think that the above check will block PRs, so it'll still be very easy to merge commits without signoff.

I can't tell if something is wrong with the way GitHub is handling this, and it's temporary, or there's something wrong with the DCO app. In the settings for this repo, I can't configure the DCO workflow as required in order to merge PRs.

hacktivist123 commented 5 months ago

Most likely something wrong with the app, it was working before I think. I'll take a look at how is done in Cilium/Cilium and replace this app

joestringer commented 5 months ago

It would be nice if that app could just work.. I think that the way we do it in cilium/cilium is more complicated and relies on a custom bot. But if that's what we need to make it work, then I'm alright with it.

The reason I bring this up is that we may want to roll out a similar mechanism across various repos within the cilium organization, so if we can "just" install the app to get this functionality then that would be the easiest path to enable it in different repos.

hacktivist123 commented 5 months ago

You have a good point, I’ll investigate this issue tomorrow and see what can be done. I’ll share my feedback/findings with you.

joestringer commented 3 months ago

DCO seems to be showing up in all the right places now. I think we can close this issue.