Skip to content

[BUG] Enable setting review notifications without delegation#3220

Open
deiga wants to merge 15 commits intointegrations:mainfrom
F-Secure-web:allow_setting_review_notifications_without_delegation
Open

[BUG] Enable setting review notifications without delegation#3220
deiga wants to merge 15 commits intointegrations:mainfrom
F-Secure-web:allow_setting_review_notifications_without_delegation

Conversation

@deiga
Copy link
Copy Markdown
Collaborator

@deiga deiga commented Feb 23, 2026 •
edited
Loading

Resolves #2273

Before the change?

  • Docs for github_team_settings we're missing computed attributes
  • Tests didn't cover all behaviour
  • Tests used legacy Check: structure
  • Resource didn't use Context-aware CRUD functions
  • Resource CRUD-functions we're tightly coupled
  • It was not possible to set an empty review_request_delegation block
  • It wasn't possible to set notify without enabling delegation

After the change?

  • Docs for github_team_settings are complete
  • Tests cover all behaviour
  • Tests use new check structures from terraform-plugin-testing
  • Resource uses Context-aware CRUD functions
  • Resource CRUD-functions are independent
  • It is possible to set an empty review_request_delegation block
  • It is possible to set notify without enabling delegation

Pull request checklist

  • Schema migrations have been created if needed (example)
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 23, 2026

👋 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 Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@deiga deiga added this to the v6.12.0 Release milestone Feb 23, 2026
@deiga deiga added the Type: Bug Something isn't working as documented label Feb 23, 2026
@deiga deiga requested a review from stevehipwell February 23, 2026 18:41
stevehipwell
stevehipwell reviewed Feb 24, 2026
View reviewed changes
Comment thread github/resource_github_team_settings.go Outdated
@deiga deiga force-pushed the allow_setting_review_notifications_without_delegation branch from 407f0ed to 7f8814d Compare February 24, 2026 20:08
@deiga deiga requested a review from stevehipwell February 24, 2026 20:08
stevehipwell
stevehipwell previously approved these changes Feb 27, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stevehipwell
Copy link
Copy Markdown
Collaborator

stevehipwell commented Mar 22, 2026

@deiga could you please rebase this?

@deiga deiga dismissed stale reviews from robert-crandall and stevehipwell via 21beff2 March 22, 2026 18:51
@deiga deiga force-pushed the allow_setting_review_notifications_without_delegation branch from 7f8814d to 21beff2 Compare March 22, 2026 18:51
@deiga
Copy link
Copy Markdown
Collaborator Author

deiga commented Mar 22, 2026

@stevehipwell @robert-crandall Rebased, please review.
I'll review the others as well, but will wait for this to be merged before asking for reviews there

stevehipwell
stevehipwell requested changes Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of things to look at.

Comment thread github/resource_github_team_settings.go Outdated
Comment thread github/resource_github_team_settings.go Outdated
@deiga deiga force-pushed the allow_setting_review_notifications_without_delegation branch from 4384422 to 28b45a8 Compare April 14, 2026 20:09
robert-crandall
robert-crandall reviewed Apr 15, 2026
View reviewed changes
Comment thread github/resource_github_team_settings.go Outdated
@deiga deiga force-pushed the allow_setting_review_notifications_without_delegation branch from dd9dcee to 48fd779 Compare April 16, 2026 18:09
@deiga
Copy link
Copy Markdown
Collaborator Author

deiga commented Apr 16, 2026

@stevehipwell Thanks. Simplified the bool checking a bit now

deiga added 3 commits April 17, 2026 10:09
@deiga
Update docs
7b2cf89 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Update tests and add missing coverage
11e0812 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add more tests to cover Import and a few more Update cases
d6506ed 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
deiga and others added 12 commits April 17, 2026 10:09
@deiga
Refactor Import to be Context-aware
92055a1 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Refactor to use Context-aware functions
61d9c2d 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Use resource.ParallelTest to make testing quicker
dde7bb7 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Remove unused call to Read
d5e6711 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Uncouple Create and Update
873ff32 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Fix resource to be able to set empty review_request_delegation block
f73417b 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add top-level notify to enable setting it without `review_request_d…
cca7c36 
…elegation`

Mark nested `notify` as deprecated

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Update docs
15f2c2a 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Consolidate getTeamSlugContext and resolveTeamIDs to use shared l…
41108e8 
…ogic with `getTeam`

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Remove named returns
a4c5128
@deiga
Fix missing reference
78e0ce5 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Simplify resolveNotify
680cacf 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga deiga force-pushed the allow_setting_review_notifications_without_delegation branch from 1bf9667 to 680cacf Compare April 17, 2026 07:10
stevehipwell
stevehipwell approved these changes Apr 17, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevehipwell stevehipwell stevehipwell approved these changes

@robert-crandall robert-crandall robert-crandall left review comments

Assignees

No one assigned

Labels

Type: Bug Something isn't working as documented

Projects

None yet

Milestone

v6.12.0 Release

Development

Successfully merging this pull request may close these issues.

[BUG]: Allow setting review notifications without delegation

3 participants

@deiga @stevehipwell @robert-crandall