Allow using GitHub Team slug instead of name via connector config option #1297
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yeah we'll need to do that so we don't break existing users :) |
tburko
force-pushed
the
use-github-team-slug-instead-of-name
branch
from
1b37a0f to
a7c998c
Compare
tburko
changed the title
|
Implemented config property |
tburko
force-pushed
the
use-github-team-slug-instead-of-name
branch
2 times, most recently
from
d617ca4 to
d5e4c7a
Compare
ericchiang
reviewed
Sep 13, 2018
connector/github/github.go
Outdated
| @@ -586,7 +594,7 @@ func (c *githubConnector) teamsForOrg(ctx context.Context, client *http.Client, | |||
|
|
|||
| for _, team := range teams { | |||
| if team.Org.Login == orgName { | |||
| groups = append(groups, team.Name) | |||
| groups = append(groups, c.teamNamesExtractor(team)...) | |||
There was a problem hiding this comment.
"teamNamesExtractor" is overkill. Just use a switch statement :) It's waaay more readable.
Documentation/connectors/github.md
Outdated
| # Acme organization would yield: | ||
| # - ['acme:Site Reliability Engineers'] for 'name' | ||
| # - ['acme:site-reliability-engineers'] for 'slug' | ||
| # - ['acme:Site Reliability Engineers', 'acme:site-reliability-engineers'] for 'both' |
There was a problem hiding this comment.
Do you have a use case for 'both'? I'd rather we drop it if not. If we ever add another field (maybe ID?) 'both' becomes a weird setting.
tburko
force-pushed
the
use-github-team-slug-instead-of-name
branch
from
c663e8e to
ccfa42b
Compare
tburko
force-pushed
the
use-github-team-slug-instead-of-name
branch
from
ccfa42b to
bf39130
Compare
mmrath
pushed a commit
to mmrath/dex
that referenced
this pull request
Sep 2, 2019
…ad-of-name Allow using GitHub Team slug instead of name via connector config option
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GitHub Team slug is arguably more script-friendly, and less surprising field to use for team names.
Compare
acme:Full-time Technical Staffusing name, andacme:full-time-technical-staffusing slug.The team slug is already being used by Terraform data module https://www.terraform.io/docs/providers/github/d/team.html for team lookups. Also https://github.com/google/go-github/ refers to teams by slug in a few places.
Our own tooling uses slug across internal systems. Not a strong argument in itself, just suspect that many other organizations are doing the same.
I have considered making the field choice configurable. Could easily add a connector Config property say
teamNameFieldto acceptname|slug|bothand default tonameto keep backwards compatibility. On the other hand, it feels like unnecessary code and flexibility. Hopefully project owners can help to make a call.