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

chore(entities): codegen cleanup #1171

Merged
merged 11 commits into from
Jun 27, 2024
Merged

chore(entities): codegen cleanup #1171

merged 11 commits into from
Jun 27, 2024

Conversation

sanderblue
Copy link
Contributor

@sanderblue sanderblue commented Jun 5, 2024

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:

@sanderblue sanderblue force-pushed the chore/codegen-cleanup branch 2 times, most recently from 2ba5f46 to 0be9f75 Compare June 7, 2024 19:16
@sanderblue sanderblue changed the title chore: codegen cleanup chore(entities): codegen cleanup Jun 12, 2024
@sanderblue sanderblue force-pushed the chore/codegen-cleanup branch from f8c7d43 to ca0ada5 Compare June 12, 2024 20:42
@sanderblue sanderblue marked this pull request as ready for review June 13, 2024 15:59
scripts/diff-types.js Outdated Show resolved Hide resolved
scripts/diff-types.js Outdated Show resolved Hide resolved
@pranav-new-relic
Copy link
Member

(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 sanderblue force-pushed the chore/codegen-cleanup branch from b5d8209 to 8e266a6 Compare June 21, 2024 19:59
@sanderblue
Copy link
Contributor Author

sanderblue commented Jun 26, 2024

@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 🤔

  • I'm not sure why UnmarshalApmApplicationEntityInterface() was deleted, but my hunch is the interface definition what it implements changed to where Tutone no longer needs to create a "special" unmarshal method for it. And if I add back the function that's currently in main, that unmarshal method references a type that doesn't exist anymore (ApmAgentInstrumentedServiceEntityOutline). And since that type was the only type referenced in the "special" unmarshal method, I think that's why we no longer need that unmarshal method now.

  • The UnmarshalJSON() still exists in several areas where it's required for special unmarshalling, but I'm fairly certain type definitions have changed so some no longer need this method I think.

  • The type EntitySummaryMetricValue no longer exists (see screenshot), so we're good there.
    Screenshot 2024-06-26 at 12 46 58 PM

  • The type DashboardWidgetCommons no longer exists as well, so I think we're also good there.

  • The AiNotificationsAuth and AiWorkflowsConfiguration types and associated methods were manually added a while back when we had to do a quick fix for a breaking change that was introduced in the API. We will want to put these types in their proper package at some point, but I didn't want to take any additional risks on top what's already being performing this massive regeneration of code.

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
Copy link
Member

pranav-new-relic commented Jun 27, 2024

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

@sanderblue sanderblue merged commit 8b2cdf4 into main Jun 27, 2024
13 of 14 checks passed
@sanderblue sanderblue deleted the chore/codegen-cleanup branch June 27, 2024 17:11
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.

4 participants