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.

Sign up for GitHub

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

Jump to bottom

MM-17053: avoid colliding with reserved team names #11705

Merged
merged 1 commit into from Jul 26, 2019
Merged

MM-17053: avoid colliding with reserved team names #11705

merged 1 commit into from Jul 26, 2019

Conversation

lieut-data
Copy link
Member

Summary

There are a handful of “reserved” team names that will be rejected if found at the start of the team name – e.g. “api21251251” or “error1251251”. Turns out we just got really unlucky and a model.NewId invocation generated just such a collision. I've changed all such random generations to include a prefix (mimicking other such tests).

Ticket Link

https://mattermost.atlassian.net/browse/MM-17053

@lieut-data
MM-17053: avoid colliding with reserved team names
1bb6d35 
There are a handful of “reserved” team names that will be rejected if found at the start of the team name – e.g. “api21251251” or “error1251251”. Turns out we just got really unlucky and a model.NewId invocation generated just such a collision. I've changed all such random generations to include a prefix.
@lieut-data lieut-data added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jul 25, 2019
@lieut-data lieut-data added this to the v5.14.0 milestone Jul 25, 2019
@lieut-data
Copy link
Member Author

@prapti, I don't think there's anything here particularly to test from QA's perspective. Just a heads up!

reflog
reflog approved these changes Jul 25, 2019
View reviewed changes
Copy link
Contributor

@reflog reflog left a comment

Choose a reason for hiding this comment

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

LTGM, one nit - maybe add a small comment before the first prefixing of "zz" explaining 'why'?

@lieut-data
Copy link
Member Author

@reflog, we actually prefix with zz_ all over the place. I wonder if we should just have a helper function to generate a random team name, and then comment in there. I'm inclined to defer that for now.

devinbinnie
devinbinnie approved these changes Jul 25, 2019
View reviewed changes
Copy link
Member

@devinbinnie devinbinnie left a comment

Choose a reason for hiding this comment

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

LGTM!

@lindalumitchell lindalumitchell removed the request for review from prapti July 26, 2019 16:29
@lindalumitchell lindalumitchell removed the 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review label Jul 26, 2019
@lieut-data lieut-data merged commit 1fa505c into master Jul 26, 2019
@lieut-data lieut-data deleted the mm-17053-avoid-reserved-team-name-collisions branch July 26, 2019 23:41
@mattermost-build mattermost-build mentioned this pull request Jul 26, 2019
Merged
@mattermod mattermod added AutomatedCherryPick and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jul 26, 2019
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation and removed 2: Dev Review Requires review by a developer labels Aug 5, 2019
@prapti prapti added the Tests/Not Needed New release tests not required label Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reflog reflog reflog approved these changes

@devinbinnie devinbinnie devinbinnie approved these changes

Assignees
No one assigned
Labels
AutomatedCherryPick Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed New release tests not required
Projects
None yet
Milestone
v5.14.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@lieut-data @reflog @devinbinnie @prapti @mattermod @lindalumitchell @amyblais