[MAINT][FEAT] Fix Org Ruleset tests && enable allowed_merge_methods
#2976
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
@stevehipwell Sure! Would you want to rebase that branch on top of go-github-v68, so that I could use it as a base for this PR? |
|
@deiga the acceptance tests PR should be compatible with the v6 releases, so the v7 branch would be rebased on it. I'm currently re-running the tests to get it ready to be merged and I'm picking up defects in the currently released code. |
deiga
force-pushed
the
fix-org-ruleset-tests
branch
from
3752c16 to
d222f43
Compare
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
I've added some minor suggestions on structuring the TF test code to make it easier to read and more idiomatic. But my main concern here is the actor ID change.
Also is there a reason you didn't add the other missing ruleset rules? Just curious.
deiga
changed the title
allowed_merge_methods
…sport` Updated the logging transport in the RateLimitedHTTPClient function to use NewLoggingHTTPTransport. As the subsystem would need to be explicitly inititated in each resource Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
It's a required field in the API and go-github doesn't use `omitempty`, so it submits `nil` if it isn't sent explicitly. This change tries to keep it in the state, without having a configuration option for it (poor choice?) And it defaults to all 3 available merge methods if it can't set something from the state. Signed-off-by: Timo Sand <[email protected]>
This is a required field and the SDK doesn't omit if it's empty Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
…on_rulesets_without_errors` to pass Signed-off-by: Timo Sand <[email protected]>
…null` Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
…_all_bypass_modes` Main fix: `bypass_actors` is returned as sorted from GH API so tests need re-indexing Signed-off-by: Timo Sand <[email protected]>
Co-authored-by: Steve Hipwell <[email protected]>
Co-authored-by: Steve Hipwell <[email protected]>
Co-authored-by: Steve Hipwell <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
With the recent changes to the SDK and the Create method we less often purely refresh state if it hasn't changed upstream, leading to us ignoring the return value of `actor_id` often Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
deiga
force-pushed
the
fix-org-ruleset-tests
branch
from
62ed5c4 to
ac1b510
Compare
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
This looks great.
LGTM
3 participants
Resolves #2530, #2536, #2717
Before the change?
TEST="./github" TESTARGS="-run TestGithubOrganizationRulesets" make testaccwould fail due to multiple issues in the test setupAfter the change?
TEST="./github" TESTARGS="-run TestGithubOrganizationRulesets" make testaccwill show all tests as passingNewSubsystemLoggingHTTPTransporttoNewLoggingHTTPTransportas we don't actually initialize a Subsystem anywhere, which creates spam.github/resource_github_organization_ruleset.goandgithub/resource_github_repository.goto use Context aware CRUD methodsrules.pull_requests.allowed_merge_methodshandling togithub/resource_github_repository_ruleset.goandgithub/resource_github_organization_ruleset.goPull 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!