search

project-everest / everest-ci

CI scripts for project everest
3 stars 8 forks source link

Port the branch-to-username feature #2

Closed msprotz closed 7 years ago

msprotz commented 7 years ago

Description: when the branch name for which we're running CI is one of the known users, send a private message to that user on Slack.

darrenge commented 7 years ago

From Node-Red, here is the list of authors:

var authors = { "ad-l": "@adl", "aseemr" : "@aseemr", "barrybo" : "@barrybo", "beurdouche" : "@beurdouche", "catalin-hritcu" : "@catalin", "fournet" : "@fournet", "jkzinzindohoue" : "@jk", "karthikbhargavan": "@karthik", "msprotz" : "@protz" , "mtzguido" : "@guido", "Nikhil Swamy" : "@nik", "parno" : "@bryan.parno", "sishtiaq" : "@sishtiaq", "s-zanella" : "@santiago", "vkanne-msft" : "@vkanne"
};

jk commented 7 years ago

@darrenge I did not contribute. You misspelled the username.

darrenge commented 7 years ago

This is just a mapping of Slack names to the emails for the CI build process.

msprotz commented 7 years ago

But actually I don't see why we should have the mapping... why not just require people to use their slack identifier, period? And we have a list of known slack members.

darrenge commented 7 years ago

good point ... maybe there was an idea to email them as well? Currently, it sends a slack message so maybe Slack ID is enough.

vkanne-msft commented 7 years ago

We needed that because people don’t have their Github usernames and slack handles coordinated. Just by taking a github username you cant necessarily send a slack message

From: darrenge [mailto:notifications@github.com] Sent: Tuesday, December 6, 2016 1:35 PM To: project-everest/everest-ci everest-ci@noreply.github.com Subject: Re: [project-everest/everest-ci] Port the branch-to-username feature (#2)

good point ... maybe there was an idea to email them as well? Currently, it sends a slack message so maybe Slack ID is enough.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fproject-everest%2Feverest-ci%2Fissues%2F2%23issuecomment-265279718&data=02%7C01%7Cvkanne%40microsoft.com%7Ca350f06f290b4cbd046608d41e1fbb27%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636166569006656933&sdata=6tLDGAqtDZCmd0pEGrMdK9bu9cPPP6yCFyacpQBci7E%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FASzQ2MJASLozJaUCkGU6L2TFOTKdZNEtks5rFdUCgaJpZM4LE_RN&data=02%7C01%7Cvkanne%40microsoft.com%7Ca350f06f290b4cbd046608d41e1fbb27%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636166569006656933&sdata=L%2BWsqLTFJ%2B59HrMZG0k87rU8L%2F60CyP%2FmilJBrHwvxo%3D&reserved=0.

msprotz commented 7 years ago

Yes, but this doesn't explain why we need to bring the github username into the picture. We could require the branch to directly use the slack username.

Two options:

vkanne-msft commented 7 years ago

The feature was originally built since devA could submit a fix into c_record and the dev wanted to get original notification. You cannot infer this from the branch name alone.

From: Jonathan Protzenko [mailto:notifications@github.com] Sent: Tuesday, December 6, 2016 1:46 PM To: project-everest/everest-ci everest-ci@noreply.github.com Cc: Sreekanth Kannepalli vkanne@microsoft.com; Comment comment@noreply.github.com Subject: Re: [project-everest/everest-ci] Port the branch-to-username feature (#2)

Yes, but this doesn't explain why we need to bring the github username into the picture. We could require the branch to directly use the slack username.

Two options:

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fproject-everest%2Feverest-ci%2Fissues%2F2%23issuecomment-265282768&data=02%7C01%7Cvkanne%40microsoft.com%7C1f9c4c27d34443409c8808d41e21573c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636166575927202503&sdata=9pUtiOU824ushpMcfjwKkJ8SjVT3i6tgzgZKXZMMOLs%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FASzQ2H3oK1DqLbLCLQwHDd5tq4T1I4bTks5rFde1gaJpZM4LE_RN&data=02%7C01%7Cvkanne%40microsoft.com%7C1f9c4c27d34443409c8808d41e21573c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636166575927212505&sdata=iywSb3iUSr9TXBAdNs3Om0L%2BKS4QAO8nHkRTkea5TG0%3D&reserved=0.

msprotz commented 7 years ago

This issue is about mapping a branch name to a username. Issue #3 is about mapping a branch name to a channel name. I'm not proposing any change for the latter, although there we could make things better.

darrenge commented 7 years ago

We have had discussion but no resolution.

Currently is there any naming convention for the branches? Such as username +"_" or anything like that?

Like you say ... we have two options: 1) Generate the slack name ("protz") from the branch name ("origin/protz_newbranch") 2) Use a hard coded list of slack user name and if we find the name in the branch name, then use it. For example, "protznewbranch" would still have protz. However, I think this opens it up to false failures as we would just be looking for a substring that could mean other things.

My vote would be a naming convention of some sort. It is easy to tell people "if you want a private message when the build completes, make sure you have a branch slackname+"_"."

msprotz commented 7 years ago

Let's go for the convention protz_feature_name with a hardcoded list of slack names in the CI script.

msprotz commented 7 years ago

That keeps the existing convention while ruling out false positives.

darrenge commented 7 years ago

Based on the Slack team directory (https://everestexpedition.slack.com/team), I get this list. I will get the name from the branch and make sure it is a valid member of the team verified by using this list. adg anitha anilam adl aozgaa aseem ashay barrybo bryan.parno dowling beurdouche dowlingbj catalin fournet chris.brzuska chris.hawblitzel wintersteiger danelahman danfab darrenge gonthier guido irinasp jaylorch jk karthik kyod konrad.kohbrok laure leodemoura polubelova markulf mlr-msft nadim nickbenton nik osavaryb wangpeng protz leino sishtiaq santiago vkanne srinath taramana thomas.sibut-pinote shaolintl vdum

darrenge commented 7 years ago

Question for @msprotz and @vkanne-msft ... what happens if we have a branch that isn't a c_ and isn't a valid Everest slack team user? Should we still send a slack message to a misc channel of some sort? I am guessing "no - in that case don't send any slack notification". This then allows the user running the script to not get a direct message or post to a slack channel if they don't want it (just by naming their branch in a non user name way).

msprotz commented 7 years ago

No I don't think we want to lose any notifications. In that case, we should just do nothing and leave the default value for the channel. That way, if someone made a typo or forgot the convention, they can still see the result of their build in the default channel (e.g. #fstar-build or #mitls-build)

vkanne-msft commented 7 years ago

Agree with Jonathan. That is how it is setup today as well.

From: Jonathan Protzenko [mailto:notifications@github.com] Sent: Tuesday, December 13, 2016 1:41 PM To: project-everest/everest-ci everest-ci@noreply.github.com Cc: Sreekanth Kannepalli vkanne@microsoft.com; Mention mention@noreply.github.com Subject: Re: [project-everest/everest-ci] Port the branch-to-username feature (#2)

No I don't think we want to lose any notifications. In that case, we should just do nothing and leave the default value for the channel. That way, if someone made a typo or forgot the convention, they can still see the result of their build in the default channel (e.g. #fstar-build or #mitls-build)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fproject-everest%2Feverest-ci%2Fissues%2F2%23issuecomment-266870540&data=02%7C01%7Cvkanne%40microsoft.com%7C52fd1a7a47ac44f161f308d423a0be3e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636172620669702247&sdata=qZ900sptdTeysxxOJxcIz%2BUZRrsrwXOEXw%2FqW0AWU18%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FASzQ2OVUc7I2sx594KlP6Zj2UmeTtvU6ks5rHxDxgaJpZM4LE_RN&data=02%7C01%7Cvkanne%40microsoft.com%7C52fd1a7a47ac44f161f308d423a0be3e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636172620669702247&sdata=IT%2BOS9wO9onJ2KJzOqzSNLTz5SfAfiY0dpigwcaJRAc%3D&reserved=0.

darrenge commented 7 years ago

Pull request has been submitted for this feature.

msprotz commented 7 years ago

Thanks, will review

darrenge commented 7 years ago

All merged and incorporated

nik commented 7 years ago

Err, I'm not part of this project ;p

vkanne-msft commented 7 years ago

Sorry it seems to have triggered because of some notes that has the @ notation

vkanne commented 7 years ago

Ditto - don't think I'm the vkanne you're looking for ;)