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

fix: make emails configurable #12

Merged
merged 2 commits into from
May 27, 2024

Conversation

fleboulch
Copy link
Contributor

Fix #11

.env.test Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/core/services/slack.ts Outdated Show resolved Hide resolved
@fleboulch fleboulch force-pushed the fix-hardcoded-emails branch 2 times, most recently from 71cf946 to 302877b Compare May 17, 2024 10:02
@greg0ire
Copy link
Member

greg0ire commented May 17, 2024

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
Copy link

pvgnd commented May 17, 2024

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
Copy link
Member

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
Copy link
Contributor Author

fleboulch commented May 17, 2024

Thanks guys for your feedbacks! I will implement it shortly

@fleboulch
Copy link
Contributor Author

fleboulch commented May 21, 2024

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

@greg0ire
Copy link
Member

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

@fleboulch
Copy link
Contributor Author

fleboulch commented May 21, 2024

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

@greg0ire
Copy link
Member

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
Copy link
Member

@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 😓

@fleboulch
Copy link
Contributor Author

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
Copy link
Member

@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.

.env.test Outdated Show resolved Hide resolved
@cicoub13
Copy link
Member

cicoub13 commented May 24, 2024

@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 ✅

Copy link
Contributor

@pfongkye pfongkye left a comment

Choose a reason for hiding this comment

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

LGTM

@greg0ire greg0ire merged commit 8fe1597 into ManoManoTech:main May 27, 2024
1 check passed
@greg0ire
Copy link
Member

Thanks @fleboulch !

@fleboulch fleboulch deleted the fix-hardcoded-emails branch May 27, 2024 12:29
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.

Users email is hardcoded
6 participants