Repository navigation
refactor: migrate resource_github_issue_label to context-aware CRUD a… - #3342
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 |
|
Please remove the resolves comment, since it adresses the issue only partly. And "resolves" will close the issue |
deiga
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for the contribution!
|
I just removed it from the description |
|
Would you be available to rebase this (possibly multiple times) during the next week? We're going to release 6.13.0 and will try to land some of these open PRs |
There was a problem hiding this comment.
Pull request overview
This PR refactors resource_github_issue_label to use Terraform Plugin SDK v2 context-aware CRUD function signatures and structured logging via tflog, aligning the resource with ongoing provider-wide migrations tracked in #2996 and #3070.
Changes:
- Migrated the resource from legacy
Create/Read/Update/DeletetoCreateContext/ReadContext/UpdateContext/DeleteContext. - Updated CRUD implementations to return
diag.Diagnosticsand wrap errors withdiag.FromErr. - Replaced
log.Printfusage withtflog.Infostructured logging and removedcontext.Background()usage.
|
Yes, I can rebase it during the next week as needed. I’ll rebase now and address the Copilot review comments as well. |
241dc85 to
40c2608
Compare
deiga
left a comment
There was a problem hiding this comment.
@stevehipwell even though there are other parts here that could be refactored, I would suggest we do the minimum here to get this merged.
stevehipwell
left a comment
There was a problem hiding this comment.
For me the bare minimum here would be to separate create & update and remove the call to read from them.
ba9fec5 to
9c162ad
Compare
|
Thanks, I separated the create and update handlers and removed the explicit read call from both paths while keeping the existing idempotent create behavior for default GitHub labels. |
9c162ad to
ad3d0c9
Compare
ad3d0c9 to
43498d3
Compare
stevehipwell
left a comment
There was a problem hiding this comment.
@slymanmrcan this is looking good. There are still a couple of outstanding comments to address and it looks like you haven't run the strict linting for new code?
88958c6 to
3fef38e
Compare
|
Thanks, my mistake. I missed the strict new-code lint locally. |
c20bfef to
9da13ec
Compare
|
Thanks for the review! I’ve pushed the fixes now: color/description are set from Read again, and Create preserves the previous GetLabel-then-EditLabel behavior for existing labels. |
|
@slymanmrcan could you please rebase the branch? |
bce3ab2 to
29f6931
Compare
stevehipwell
left a comment
There was a problem hiding this comment.
As per my previous comment, this resource should fail on create if the label already exists. You can either import or use github_isue_labels to handle the existing label scenario.
e4ff06a to
27f907c
Compare
|
thanks for review
|
27f907c to
e637931
Compare
|
@slymanmrcan could you please fix the lint failures? |
e637931 to
b82248d
Compare
|
Thanks, fixing it now. |
|
@slymanmrcan you appear to have been caught out by a regression. @deiga any thoughts? |
robert-crandall
left a comment
There was a problem hiding this comment.
Thanks for this! The migration to context-aware CRUD with diag and tflog is clean and consistent, and I really appreciate how quickly you turned around the feedback. The color/description read-back restores drift detection, and setting the description unconditionally so it can be cleared is exactly right. Nice, well-scoped change. 🙏
Addresses parts of #3070
Addresses parts of #2996
Before the change?
resource_github_issue_labeluses legacyCreate/Read/Update/Deleteschema functions and
log.Printffor logging.After the change?
CreateContext/ReadContext/UpdateContext/DeleteContext(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnosticslog.Printfwithtflog.Infofor structured loggingcontext.Background()callsdiag.FromErr()Pull request checklist
Does this introduce a breaking change?