search

kubernetes-sigs / slack-infra

Tooling for kubernetes.slack.com
Apache License 2.0
90 stars 36 forks source link

Changing len frm 9 to 11 to match new string count #31

Closed markjacksonfishing closed 4 years ago

markjacksonfishing commented 4 years ago

Slack has changed the unique member ID from 9 to 11 characters. Over the weekend this test failed This is the pr Signed-off-by: markyjackson-taulia marky.r.jackson@gmail.com

phumberdroz commented 4 years ago

Old ID's should not be affected this would not fix the bug and would introduce a new one because now the test case would fail with old ID's (9 long)

markjacksonfishing commented 4 years ago

Old ID's should not be affected this would not fix the bug and would introduce a new one because now the test case would fail with old ID's (9 long)

aha! Good catch. So the logic for the unique ID needs to be rethought

markjacksonfishing commented 4 years ago

After chatting with @phumberdroz this should start with either U or W but can have an undefined number of digits after that is the safer case. The slack official spec states:

Identifier for this workspace user. It is unique to the workspace containing the user. Use this field together with team_id as a unique key when storing related data or when specifying the user in API requests. We recommend considering the format of the string to be an opaque value, and not to rely on a particular structure.

So I am going to put this on hold and rework this. /hold

markjacksonfishing commented 4 years ago

/hold cancel

markjacksonfishing commented 4 years ago

/assign @jeefy @BenTheElder

markjacksonfishing commented 4 years ago

@Katharine i am going to have a subsequent pr to have the check have unlimited digits but look for U or W. Yeah, this addresses the short term need

jeefy commented 4 years ago

@markyjackson-taulia Could you add tests that validate both 9 and 11 length? And also a test that validates it fails with another length?

lgtm otherwise <3

k8s-ci-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, Katharine, markyjackson-taulia

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/slack-infra/blob/master/OWNERS)~~ [Katharine] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
cpanato commented 4 years ago

/hold for others to review

jeefy commented 4 years ago

lgtm /hold cancel