Skip to content

feat: allow relying on manually defined user ID when resolving users/email#954

Open
GerkinDev wants to merge 4 commits intojenkinsci:masterfrom
GerkinDev:feat/allow-manual-definition-of-slack-id
Open

feat: allow relying on manually defined user ID when resolving users/email#954
GerkinDev wants to merge 4 commits intojenkinsci:masterfrom
GerkinDev:feat/allow-manual-definition-of-slack-id

Conversation

@GerkinDev
Copy link
Copy Markdown

@GerkinDev GerkinDev commented Apr 2, 2024

Change the behavior of EmailSlackUserIdResolver::resolveUserIdForEmailAddress, to allow sending notifications to users who's slack email is not their Jenkins email

  • Before:
    • Query the slack API for the given email address
    • If the email address is not found in slack, directly return null
  • After
    • Query the slack API for the given email address
    • If not found, try to infer the username (usually the part before the @ in the email address, as seen in my local Jenkins install), and fetch that user's Slack ID
    • If not found, find all users with the given email and extract their Slack ID

Testing done

Added some tests.
Tried to resolve a user via auto infered username from Jenkins pipeline

### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [ ] Link to relevant issues in GitHub or Jira
- [ ] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

@GerkinDev GerkinDev requested a review from a team as a code owner April 2, 2024 11:14
@jetersen
Copy link
Copy Markdown
Member

jetersen commented Apr 2, 2024

@GerkinDev can you fix the spotbugs issues?

@GerkinDev GerkinDev force-pushed the feat/allow-manual-definition-of-slack-id branch 3 times, most recently from 351b8ec to c6affc4 Compare April 2, 2024 22:13
@GerkinDev
Copy link
Copy Markdown
Author

Done !

@GerkinDev GerkinDev force-pushed the feat/allow-manual-definition-of-slack-id branch from c6affc4 to aa74955 Compare April 8, 2024 20:34
@GerkinDev
Copy link
Copy Markdown
Author

Hi, can I have any info about if you're interested in such change, or if you want to change some behavior? My organization would greatly benefit from this, since we are almost unable to ping any user because our github email addresses is not the same as our slack addresses.

If you have any suggestions about how we could make it work without this change, I would gladly take it.

As an alternative, I've imagined adding a global map, mapping addresses to slack ids out of user profiles, but it would add a whole new logic.

Copy link
Copy Markdown
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Hi,

I don't really want this in the main resolver as its just guessing based on organisation policies, (I find it quite odd slack and SCM emails aren't matching anyway =/)

Options I see:

  1. users can currently set their Slack ID on their profile and that will be used. This may not be great for scaling if an admin wants to do this centrally.
  2. create a global list somehow, or maybe add a url to fetch a JSON document from? (I've done this before but in a pipeline library), (probably as another SlackUserId resolver)
  3. add a new SlackUserId resolver (ideally in your own custom plugin but could maybe be here) doing this.

@GerkinDev
Copy link
Copy Markdown
Author

GerkinDev commented May 13, 2024

Thank you for the feedback. I'll look for the 1st solution you've suggested. If we cannot achieve the behavior we want with it, I'll open a new PR with a new resolver implementing 2. & 3. I don't feel like creating a new plug-in, since no one in my team would be able to maintain it.

You can close this PR if you want

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants