[BUG] Enable importing github_emu_group_mapping for Group with multiple teams
#3054
Conversation
|
👋 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 |
deiga
changed the title
github_emu_group_mapping for Group with multiple teams
deiga
force-pushed
the
fix-emu-group-mapping-import-multiple-teams-3030
branch
from
beb7f4d to
abb3de3
Compare
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
As we as the inline comments, we need to be careful when using tflog that it can only be used with contexts injected by TF (e.g. CreateContext).
| tflog.Debug(ctx, "Parsed two-part import ID", map[string]any{ | ||
| "import_id": importID, | ||
| "group_id": groupID, | ||
| "team_slug": teamSlug, | ||
| }) | ||
|
|
||
| tflog.Trace(ctx, "Setting state attribute: group_id", map[string]any{ | ||
| "group_id": groupID, | ||
| }) |
There was a problem hiding this comment.
Why do we need both of these?
There was a problem hiding this comment.
I liked the idea of having a DEBUG statement "Parsing successful" and separately TRACE statements for each operation that isn't covered and can lead to an interruption.
But that might be too verbose and unnecessary. I'm trying to think about how we can get better information from future DEBUG or TRACE logs from bug reports 😬
There was a problem hiding this comment.
I don't think there is value in two log entries without anything between them.
There was a problem hiding this comment.
Even if they are for different log levels?
There was a problem hiding this comment.
Even if they are for different log levels?
Yes, as long as the payload isn't significantly different for the lower level log. Logs need to have a purpose and there isn't a purpose for logging to two different messages at the same point; it doesn't make it any easier for me to debug the system and arguably it makes it harder.
In this case the higher level log (trace) actually contains a smaller payload than the lower level one.
| tflog.Debug(ctx, "Parsed integer import ID", map[string]any{ | ||
| "import_id": importID, | ||
| "group_id": groupID, | ||
| }) | ||
|
|
||
| tflog.Trace(ctx, "Setting state attribute: group_id", map[string]any{ | ||
| "group_id": groupID, | ||
| }) |
There was a problem hiding this comment.
Why do we need both of these?
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Which was found by our new tests! 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>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
deiga
force-pushed
the
fix-emu-group-mapping-import-multiple-teams-3030
branch
from
abb3de3 to
a523161
Compare
2 participants
Superseded #3043
Resolves #3030
Before the change?
github_emu_group_mappingAfter the change?
github_emu_group_mappingPull request checklist
Schema migrations have been created if needed (example)Does this introduce a breaking change?
Please see our docs on breaking changes to help!