-
Notifications
You must be signed in to change notification settings - Fork 472
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: prevent creating teams with reserved team names #21727
Conversation
Note: based on this code the requirement in the bug ticket for something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FE looks good 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, left a question about consistent error messages
ee/server/service/teams.go
Outdated
return nil, fleet.NewInvalidArgumentError("name", "may not be all teams") | ||
} | ||
if l == strings.ToLower(fleet.ReservedNameNoTeam) { | ||
return nil, fleet.NewInvalidArgumentError("name", "may not be no team") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dumb question, why these have a different error message? the two below look better:
"All teams" is a reserved team name`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roperzh I think I was just trying to be consistent with the other errors in this part of the code. I can update it to be consistent with the other errors I added though.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #21727 +/- ##
==========================================
- Coverage 64.84% 64.84% -0.01%
==========================================
Files 1500 1500
Lines 119108 119123 +15
Branches 3502 3509 +7
==========================================
+ Hits 77240 77249 +9
- Misses 34858 34861 +3
- Partials 7010 7013 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! As a nit you can totally ignore:
the error string is repeated many times, for similar cases we tend to declare a variable (there's a list of errors somewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just thinking about this because I'm working on policies for "No team". And you can specify Team: "<TeamName>"
for policies when applying them via fleetctl apply
.
Should we do a migration to rename existing teams with the "No team" name?
I was thinking about this as well! What would we rename them to? |
I thought it was informally discussed we won't try to migrate old teams bye I might be misremembering |
Yeah, it makes sense it would break some workflows (e.g. gitops). |
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/
,orbit/changes/
oree/fleetd-chrome/changes
.See Changes files for more information.