Repository navigation
fix: Allow removing runner group networks - #4541
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Add explicit removal flags for organization and enterprise runner group updates, preserving existing omission and string behavior.
9788c28 to
c4d4c88
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4541 +/- ##
=======================================
Coverage 98.61% 98.61%
=======================================
Files 199 199
Lines 18675 18693 +18
=======================================
+ Hits 18417 18435 +18
Misses 258 258 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@austenstone - just like in another recent PR, we now have a CLA problem because an agent signed a commit that was pushed to this PR. So now, you can either clean up the commits locall and force-push the changes to this PR to get rid of the bot's commits or you can close this PR and open a brand new one. Your choice, but we cannot continue this PR with the current CLA state, unfortunately. |
Keep explicit JSON null removal separate from empty-string compatibility. Document the primitive-pointer encoding contract and exercise the removal flag through organization and enterprise update requests.
b0fc528 to
6cf8993
Compare
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @austenstone.
One wording suggestion, otherwise, LGTM.
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @austenstone!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
stevehipwell
left a comment
There was a problem hiding this comment.
The code changes look good but I don't think the contributing changes are quite right.
gmlewis
left a comment
There was a problem hiding this comment.
@austenstone - please fix the linter and test errors and push the changes to this PR and we can then continue.
Merge current master and adapt organization runner-group tests to its required string name field. Keep explicit null removal coverage and verify zero-value name serialization.
|
Fixed in 50d7554. Merged current master and updated the organization runner-group fixtures for the required string name, including the expected JSON bodies. Explicit null removal coverage is unchanged. Full |
gmlewis
left a comment
There was a problem hiding this comment.
@stevehipwell - have all your comments been addressed now?
|
Thank you, @stevehipwell! |
Add
RemoveNetworkConfigurationto organization and enterprise runner-group update requests, followingUpdateTeamRequest.RemoveParentTeam. Setting it sends"network_configuration_id": nulland takes precedence over a supplied ID. The value-receiver marshaler supports both values and pointers without mutation. Creates and responses are unchanged.The Go 1.26.0
encoding/jsonrepro shows that*stringbehaves identically withomitemptyandomitzero: nil omits the field,new("network-id")sends a string, andnew("")sends"". Neither tag produces JSON null. Empty-string behavior is preserved for compatibility, but empty-string detachment is unverified. Both PATCH schemas allow null: organization and enterprise.Regression tests cover omission, assignment, empty-string compatibility, explicit null, flag precedence, preservation of other fields, value/pointer marshaling, request immutability, and both native PATCH bodies. The null cases fail without the marshalers. On Go 1.26.0, runner-group race tests, all-module
script/test.shrace tests,script/fmt.sh,script/generate.sh,script/lint.sh, and OpenAPI metadata validation pass. No live API writes were made for this revision.This revises the earlier
omitzeroimplementation and still needs maintainer agreement on explicit-null removal. Needed by integrations/terraform-provider-github#3274, which remains gated on a merged and released SDK fix.Copilot assisted with implementation, tests, and this description.