search

ManoManoTech / homer

Homer is a Slack bot intended to help you to easily share and follow Gitlab merge requests.
MIT License
29 stars 8 forks source link

fix: make emails configurable #12

Closed fleboulch closed 3 months ago

fleboulch commented 3 months ago

Fix #11

greg0ire commented 3 months ago

The repository has no tags yet, and this is a breaking change for ManoMano. I guess we should tag it as 1.0.0 (or maybe 0.1.0, and we tag 1.0.0 when we know for a fact another company is using it. @fleboulch , are you already using it?) and document the breaking change in the release notes. I will try to find out what is our deploy process internally to determine what our deploy process for Homer is, because we need to define that variable somewhere before deploying the new release.

pvgnd commented 3 months ago

The repository has no tags yet, and this is a breaking change for ManoMano. I guess we should tag it as 1.0.0 (or maybe 0.1.0, and we tag 1.0.0 when we know for a fact another company is using it. @fleboulch , are you already using it?) and document the breaking change in the release notes. I will try to find out what is our deploy process internally to determine what our deploy process for Homer is, because we need to define that variable somewhere before deploying the new release.

Hey @greg0ire ! I hope your are doing well. In order to avoid the breaking change for now, you can setup a default value for the email patterns keeping the ManoMano value. So @fleboulch will be able to use it. And then feel free to remove the default value when ManoMano will be ready internally

greg0ire commented 3 months ago

Hey mate, doing well, thanks! I asked internally about the deploy process… no answer so far, so yeah, let's do as you suggest. @fleboulch, can you please implement a fallback to the legacy values?

fleboulch commented 3 months ago

Thanks guys for your feedbacks! I will implement it shortly

fleboulch commented 3 months ago

I added a commit to implement the fallback behaviour. Feel free to comment

greg0ire commented 3 months ago

I will try to get Js specialists at ManoMano to review this, as I'm not one of them by any means.

fleboulch commented 3 months ago

Thanks a lot! I'm not also a JS expert so maybe there is some parts to improve

greg0ire commented 3 months ago

I'm starting to think we are still using our internal fork, as MRs keep flowing to it, so I might ask you to git reset --hard HEAD^ && git push --force at some point, just trying to get an official confirmation on this.

greg0ire commented 3 months ago

@fleboulch I've been able to confirm that we are still not using the open-source version of Homer internally, which means this won't be a breaking change. You can drop that last commit with the instructions in my previous message. Sorry for wasting your time with this :sweat:

fleboulch commented 3 months ago

I removed it. I'm seeing a step in red because my commit is not signed. Is it a mandatory policy?
I've never seen this policy in other open source projects but I can sign it if you really need this

greg0ire commented 3 months ago

@cicoub13 do you recall why signed commits are a requirement? I'm not saying Homer isn't a sensitive project (after all, it has access to Gitlab and Slack), but I'm not sure how much sense it makes in the context of open source: it just guarantees that somebody is not trying to pass as somebody else, but since we don't particularly do a background check on people in the first place, I don't see the point.

cicoub13 commented 3 months ago

@cicoub13 do you recall why signed commits are a requirement?

We required that for internal/private repositories. I don't see any reason for public ones (as you said, we cannot require identity verification for contributors). I removed it for this repository ✅

greg0ire commented 3 months ago

Thanks @fleboulch !