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

Ddebko ICU-2122 Enforce Typed Definitions In API Module #2238

Merged
merged 29 commits into from
Jul 21, 2022

Conversation

ddebko
Copy link
Collaborator

@ddebko ddebko commented Jun 28, 2022

Issue-2122

Summary:

The focus for issue 2122 is to add typed definitions to our return methods in the API module. Currently we are returning generic types (interface{}), which requires the user to type cast the value into the expected typed definition. Otherwise, the values stored in the generic type are hidden. This PR adds the typed definitions to the API module by updating the template in the genapi package. The changes to the API module created a cascading error effect in the cli commands, which is why I listed many items in the changes section for the cli module.

Changes:

  • Updated apigen module template
    • Changed return type for ReadResult.GetItem() from interface{} to typed definition
    • Changed return type for ListResult.GetItems() from interface{} to typed definition
    • Delete interface definitions: GenericResult, GenericDeleteResult, GenericListResult
  • Updated cligen module template
    • removed references to typed interface definitions: GenericResult, GenericDeleteResult, GenericListResult
    • added typed references
      • *api.Response
      • *{{ .PackageName }}.{{ Resource Type }}
        • Ex.) *authtokens.AuthToken
      • []*{{ .PackageName }}.{{ Resource Type }}
        • Ex.) []*authtokens.AuthToken
      • *{{ .PackageName }}.{{ Resource Type }}ReadResult
        • Ex.) *authtokens.AuthTokenReadResult
      • *{{ .PackageName }}.{{ Resource Type }}DeleteResult
        • Ex.) *authtokens.AuthTokenDeleteResult
      • *{{ .PackageName }}.{{ Resource Type }}ListResult
        • Ex.) *authtokens.AuthTokenListResult
    • updated cli helper func executeExtraActions
      • removed parameter "origResult api.GenericResult"
      • added parameters/return types:
        • origResp *api.Response
        • origItem *{{ .PackageName }}.{{ Resource Type }}
        • origItems []*{{ .PackageName }}.{{ Resource Type }}
          • note this is only generated when a command has a Result type with a method GetItems()
    • updated cli helper funcs PrintJsonItem & PrintJsonItems to pass in *api.Response in format.go
    • updated all helper funcs in files labels funcs.go in gencli package
      • updated executeExtraActionsImpl to match func definition in generated files
      • fixed printItemTable by a case to case situation depending on the command

Concerns:

  • In the API module we have a DeleteResult type that always returns a nil value for GetItem and we have a comment for the functions to explain this, therefore I decided not to change the return type.
  • This is a breaking change for users that are currently using the API Module

ddebko added 23 commits June 21, 2022 10:34
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good. Could you also prepare some notes in the CHANGELOG that detail the exact breaking changes for an external user of the api module?

internal/cmd/gencli/templates.go Outdated Show resolved Hide resolved
tmessi
tmessi previously approved these changes Jul 5, 2022
…2122-fix-sdk-template

# Conflicts:
#	CHANGELOG.md
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase this on the v0.9.1 release changelog changes

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but wait for Louis and Mike

louisruch
louisruch previously approved these changes Jul 18, 2022
Copy link
Collaborator

@louisruch louisruch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ddebko ddebko merged commit c2bfcc0 into main Jul 21, 2022
@ddebko ddebko deleted the ddebko-ICU-2122-fix-sdk-template branch July 21, 2022 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants