feat!: Change AuditEntry.Org and AuditEntry.OrgID to json.RawMessage

[antixcode6]

BREAKING CHANGE: AuditEntry.Org and AuditEntry.OrgID are now json.RawMessage with new getters.

Fixes: #3488.

This PR is meant to resolve the issues around #3488

I am modifying AuditEntry.Org and AuditEntry.OrgID from *string and *int64, respectively, to both be json.RawMessage types this is meant to allow the consumer of this client to test the fields for being a string/int64 or being a []string/[]int64 and handle it accordingly.

This PR also modifies the tests TestOrganizationService_GetAuditLog TestAuditEntry_Marshal and TestEnterpriseService_GetAuditLog to ensure functionality remains after changing the above mentioned fields to json.RawMessage

I have also added two methods, AuditEntry.GetOrgID and AuditEntry.GetOrg, to skipStructMethods and ran the code gen to remove the tests and methods to make sure we do not need to modify the generated tests/accessor methods.

[google-cla]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.30%. Comparing base (56f5036) to head (4076945).
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3502      +/-   ##
==========================================
+ Coverage   91.21%   91.30%   +0.09%     
==========================================
  Files         182      183       +1     
  Lines       15930    16095     +165     
==========================================
+ Hits        14531    14696     +165     
  Misses       1225     1225              
  Partials      174      174              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

@antixcode6 - this looks like a great start! Why not add the new getter methods to this PR to keep it all in the same place?

I'm thinking that you can add replacements for the original AuditEntry.GetOrgID and AuditEntry.GetOrg methods with the same result types as before (but with the added ok field if it is indeed the expected type), and then add two new methods with your new getters that support the alternative type(s) that the fields represent.

NOTE that your new getters would live right next to the struct defs or method defs... please do NOT put them in the auto-generated files, as they will be constantly overwritten.

Thoughts?

[antixcode6]

@gmlewis sounds good! I just pushed up some changes with four getters, two raw getters and two for the same result types as before with the added ok.

Also I created a test to make sure these getters are covered.

[gmlewis]

Thank you, @antixcode6!
Did you want to add new getters for the other types being returned that you discovered?

Otherwise, LGTM.

[antixcode6]

@gmlewis this covers both Org and OrgID which are the two I discovered to be returning arrays sometimes, otherwise I haven't noticed anything else that would need to be modified

[gmlewis]

@gmlewis this covers both Org and OrgID which are the two I discovered to be returning arrays sometimes, otherwise I haven't noticed anything else that would need to be modified

So you don't want to add getters that return ([]string, bool) and ([]int64, bool) respectively?

[antixcode6]

@gmlewis good suggestions I've made the changes, I also had to fix something in my test case. All should be good now

[gmlewis]

Thank you, @antixcode6!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@stevehipwell - might you have time for a code review? Thank you!

[stevehipwell]

The implementation looks good, but I'm not a fan of json.RawMessage. Did you consider using a custom marshaller and adding Orgs & OrgIDs to take the array versions? I did this or something similar for the ruleset APIs.

[gmlewis]

The implementation looks good, but I'm not a fan of json.RawMessage. Did you consider using a custom marshaller and adding Orgs & OrgIDs to take the array versions? I did this or something similar for the ruleset APIs.

Do you want to create an alternative PR and then we can look at the two implementations and decide which one we prefer?

[stevehipwell]

@gmlewis sorry but I don't think I have the bandwidth to pick up a PR for this now. This is probably a wider discussion than just this PR as GitHub APIs often include types that can't be directly represented by the Go type system.

After re-reading the issue I have a slightly different view. The schema is incorrect, which needs to be reported at github/rest-api-description (but I wouldn't hold your breath as it looks as much of a ghost town as the TF provider). And for the implementation as we don't need to directly follow the GitHub API, I think the struct should have Orgs & OrgIDs fields both as arrays; these can be marshalled from either a single string or an array to result in a consistent representation where the end user doesn't need to understand how to extract the data.

[gmlewis]

After re-reading the issue I have a slightly different view. The schema is incorrect, which needs to be reported at github/rest-api-description (but I wouldn't hold your breath as it looks as much of a ghost town as the TF provider). And for the implementation as we don't need to directly follow the GitHub API, I think the struct should have Orgs & OrgIDs fields both as arrays; these can be marshalled from either a single string or an array to result in a consistent representation where the end user doesn't need to understand how to extract the data.

I'm open to this suggestion. Are you, @antixcode6?

[antixcode6]

@gmlewis @stevehipwell I'm open to this, would it make sense to open a separate PR with with the Orgs and OrgIDs fields as part of the struct, and then we can pick the more appropriate change? I have no problem creating a new branch and PR but if it is better to work out of here I am ok with that too.

[gmlewis]

@gmlewis @stevehipwell I'm open to this, would it make sense to open a separate PR with with the Orgs and OrgIDs fields as part of the struct, and then we can pick the more appropriate change? I have no problem creating a new branch and PR but if it is better to work out of here I am ok with that too.

Sure, a separate PR makes sense to me. Thank you!