Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add organizationRevokeSharedAccount and accountManagementCancelAccount mutations #1187

Conversation

iamsumit
Copy link
Contributor

@iamsumit iamsumit commented Jul 8, 2024

We need these mutations for the project we're implementing this so adding it as PR. Thanks!

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2024

CLA assistant check
All committers have signed the CLA.

@iamsumit iamsumit force-pushed the account-management-org-shared-account-api branch from 7888c00 to f3630e1 Compare July 8, 2024 08:37
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 51.42857% with 17 lines in your changes missing coverage. Please review.

Project coverage is 38.40%. Comparing base (3ff4ee2) to head (bdec0ed).
Report is 66 commits behind head on main.

Files Patch % Lines
pkg/accountmanagement/accountmanagement_api_.go 0.00% 13 Missing ⚠️
pkg/accountmanagement/accountmanagement_api.go 81.81% 1 Missing and 1 partial ⚠️
pkg/organization/organization_api.go 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1187      +/-   ##
==========================================
- Coverage   38.84%   38.40%   -0.44%     
==========================================
  Files          86       96      +10     
  Lines        5612     4822     -790     
==========================================
- Hits         2180     1852     -328     
+ Misses       3266     2795     -471     
- Partials      166      175       +9     
Flag Coverage Δ
unit 38.40% <51.42%> (-0.44%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@pranav-new-relic pranav-new-relic changed the title Add organizationRevokeSharedAccount and accountManagementCancelAccount mutation. feat: add organizationRevokeSharedAccount and accountManagementCancelAccount mutations Jul 10, 2024
@pranav-new-relic
Copy link
Member

pranav-new-relic commented Jul 10, 2024

Hi @iamsumit and @coderkakarrot, thank you for submitting these additions to the New Relic Go Client. While all of the changes you've made are exactly on par with what one is supposed to do via Tutone, we've noticed a few discrepancies with the generated code (upon trying this myself), in response to which I had to make a couple of other changes to your code - just wanted to share these with you for context. This note shall also be useful for maintainers of the New Relic Go Client to take note of.

  • It may be obtained that in the accountmanagement package, alongside the new mutation you guys have added, changes have been made to the GetManagedAccounts() function to take isCanceled as an argument to the function, as this is now an optional variable used in the query. However, Tutone doesn't seem to be updating the query with this boolean variable, probably missing out on this because it is an optional variable.
  • Along with the above, as the GetManagedAccounts() function now has an additional argument which needs to be specified, which would result in accounts being filtered by isCanceled false or true, in order to not inflict breaking changes on dependent upstream services such as utilities managing account management in the New Relic Terraform Provider, we would need to convert isCanceled bool into isCanceled *bool, which also is currently unsupported by Tutone.

Owing to the above, we've moved duplicated code of the GetManagedAccounts() function with the above changes to a new file accountmanagement_api_.go, while accountmanagement_api.go would continue to have GetManagedAccounts() not yet updated via Tutone, so we have this causing no probable impact and you guys are unblocked too, while we evaluate if this can be brought back into Tutone scope sometime in future. Hence, if you'd like to use isCanceled with GetManagedAccounts(), please use the version of this function in accountmanagement_api_.go, GetManagedAccountsWithAdditionalArguments() for time being.

Rest assured, the functions for the new mutations you've added one each in accountmanagement and organization are untouched and shall be merged as they are.

I shall proceed with merging this PR and releasing these changes; shall keep you guys posted if we find a way sometime to bring accountmanagement_api_.go back into Tutone scope.

@pranav-new-relic pranav-new-relic merged commit 396df9d into newrelic:main Jul 10, 2024
12 of 13 checks passed
@pranav-new-relic
Copy link
Member

pranav-new-relic commented Jul 10, 2024

@iamsumit FYI: these changes have just been released with v2.41.0 of the New Relic Go Client 🎉

@iamsumit
Copy link
Contributor Author

@pranav-new-relic Thank you, @pranav-new-relic for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants