Skip to content

[BUG] Enable setting review notifications without delegation#3220

Open
deiga wants to merge 13 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 13 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
@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 added 12 commits March 22, 2026 20:30
@deiga
Update docs
a11a9d8 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Update tests and add missing coverage
90cc855 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add more tests to cover Import and a few more Update cases
f62ee13 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Refactor Import to be Context-aware
0e5c8db 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Refactor to use Context-aware functions
425a8ba 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Use resource.ParallelTest to make testing quicker
a68e71a 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Remove unused call to Read
bcac949 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Uncouple Create and Update
aafb1f8 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Fix resource to be able to set empty review_request_delegation block
8b05e36 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add top-level notify to enable setting it without `review_request_d…
2c6774d 
…elegation`

Mark nested `notify` as deprecated

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

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@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.

github/resource_github_team_settings.go
// only one source can be active at a time.
func resolveNotify(ctx context.Context, d *schema.ResourceData) bool {
// Check if top-level notify is explicitly configured.
if v, ok := d.GetOk("notify"); ok {
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell Mar 27, 2026

Choose a reason for hiding this comment

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

I'm not sure there is any value to using GetOk() here given the way it works and the fact that you're using an explicit schema?

Copy link
Copy Markdown
Collaborator Author

@deiga deiga Mar 27, 2026

Choose a reason for hiding this comment

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

The way I understand the code is that this is exactly where GetOk() should work for TypeBool.
It should check if notify has been set in the config, right? Of course the tests don't have that scenario at all 🙃

Copy link
Copy Markdown
Collaborator Author

@deiga deiga Mar 27, 2026

Choose a reason for hiding this comment

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

Or how else would I be checking which one of the fields is actually set? Should I just read both and see if either is true? That sounds simpler 🤔

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

Reviewers

@stevehipwell stevehipwell stevehipwell requested changes

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

Requested changes must be addressed to merge this pull request.

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