[MAINT] Consolidate ID format of github_emu_group_mapping#3215
[MAINT] Consolidate ID format of github_emu_group_mapping#3215deiga wants to merge 5 commits intointegrations:mainfrom
github_emu_group_mapping#3215Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
This PR looks good, it's just the discussion about the log context to resolve.
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
deiga
force-pushed
the
consolidate-emu-group-mapping-id
branch
from
3480f37 to
472d4d6
Compare
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
I think we can remove a bit more complexity from the logging.
| tflog.Debug(ctx, "Querying external groups linked to team from GitHub API", map[string]any{ | ||
| "org_name": orgName, | ||
| "team_slug": teamSlug, | ||
| }) |
There was a problem hiding this comment.
I don't think you need these fields as you've already got the ID and this just duplicates the ID parts.
| "configured_group_id": groupID, | ||
| "upstream_group_id": group.GetGroupID(), | ||
| "group_name": group.GetGroupName(), |
There was a problem hiding this comment.
I don't think these are required here, ID is already in context and if something doesn't match there is a log line below to show this.
| "org_name": orgName, | ||
| "team_slug": teamSlug, | ||
| "group_id": groupID, |
2 participants
Resolves #3200
Before the change?
tflog.SetFieldwas being used incorrectlyterraform-plugin-testingAfter the change?
tflog.SetFieldusageterraform-plugin-testingPull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!