newrelic / newrelic-client-go

New Relic Client for the Go programming language
https://newrelic.github.io/observability-as-code
Apache License 2.0
73 stars 89 forks source link

chore(entities): codegen cleanup #1171

Closed sanderblue closed 1 week ago

sanderblue commented 1 month ago

Our entities package (along with a few others), contain a mix of manually edited code and generated code. The primary intent of this PR is to clean up and reorganize our entities package in a manner that allows us to generate code seamlessly for this package.

This PR will require changes in terraform-provider-newrelic and newrelic-cli to accommodate updates to inputs and types. These changes should not be breaking changes.

Important design patterns to discuss as a team:

  1. The file names of files that contain manually edited code now end with an underscore (_). E.g. entities/entities_api_.go and entities/types_.go. We have the option to flip this as well, i.e. make the generated files have the underscore, but that would require a massive overhaul with our code generation across the entire repo.
  2. Certain types that existed in the entities package, such as some Dashboard types, really ought to live in their respective packages. There will be caveats, but this will be improve scalability as we move towards maximum code generation.


Associated PRs for dependent projects:

pranav-new-relic commented 2 weeks ago

(edited version of the comment posted on 19 Jun 2024)

@sanderblue one more question 😄 in entities/types.go, looks like Tutone has been deleting a couple of interface unmarshal methods such as func UnmarshalApmApplicationEntityInterface(), func UnmarshalApmApplicationEntityOutlineInterface() and changes around these functions (there are other references of deleted unmarshal functions UnmarshalJSON too, along with other things like func UnmarshalDashboardWidgetCommonsInterface(), func UnmarshalEntitySummaryMetricValueInterface() and similar ones but I believe these are mostly related to outdated types we have had for a very long time in the entities package).

Can you please take a look just so we can double check removing these methods is safe? I plan on taking a deeper look into entities/types.go tomorrow when I'm back again, but just wanted to know your opinion about this.

I see func UnmarshalAiWorkflowsConfigurationInterface() and func UnmarshalAiNotificationsAuthInterface() have also been removed from types.go, but moved to types_.go, which is why I'm wondering the ones I've listed above also need to be in types_.go maybe? Just a thought 🤔

sanderblue commented 1 week ago

@pranav-new-relic

(edited version of the comment posted on 19 Jun 2024)

@sanderblue one more question 😄 in entities/types.go, looks like Tutone has been deleting a couple of interface unmarshal methods such as func UnmarshalApmApplicationEntityInterface(), func UnmarshalApmApplicationEntityOutlineInterface() and changes around these functions (there are other references of deleted unmarshal functions UnmarshalJSON too, along with other things like func UnmarshalDashboardWidgetCommonsInterface(), func UnmarshalEntitySummaryMetricValueInterface() and similar ones but I believe these are mostly related to outdated types we have had for a very long time in the entities package).

Can you please take a look just so we can double check removing these methods is safe? I plan on taking a deeper look into entities/types.go tomorrow when I'm back again, but just wanted to know your opinion about this.

I see func UnmarshalAiWorkflowsConfigurationInterface() and func UnmarshalAiNotificationsAuthInterface() have also been removed from types.go, but moved to types_.go, which is why I'm wondering the ones I've listed above also need to be in types_.go maybe? Just a thought 🤔

Phew, that was a lot to go through, but I'm glad you called all that stuff out. It gives me some more confidence that this huge code regeneration should work out okay. 🙂 😎 🤞

pranav-new-relic commented 1 week ago

Thanks for clarifying - I just took a final look and I guess we're all set :)