ddelnano / terraform-provider-mikrotik

MIT License
131 stars 29 forks source link

Code generation #150

Closed maksym-nazarenko closed 1 year ago

maksym-nazarenko commented 1 year ago

Closes #47

This PR replaces #104.

Complete before merge:

What is the value of this PR?

It introduces new tool mikrotik-codegen, that supports codegen struct tag to generate Terraform resource based of struct definition in client/ file.

The main purpose of this tool is to provide a fast and easy way to start working on new resource.

Warning

This tool is not supposed to be used as automated way to generate resource without human review. It just creates >90% of boilerplate code.

How to use new feature?

Example usage might look like:

type MikrotikResource struct{
    Id             string   `mikrotik:".id"             codegen:"id,mikrotikID,deleteID"`
    Name           string   `mikrotik:"name"            codegen:"name,required,terraformID"`
    Enabled        bool     `mikrotik:"enabled"         codegen:"enabled"`
    Items          []string `mikrotik:"items"           codegen:"items,elemType=string"`
    UpdatedAt      string   `mikrotik:"updated_at"      codegen:"updated_at,computed"`
    Unused         int      `mikrotik:"unused"          codegen:"-"`
    NotImplemented int      `mikrotik:"not_implemented" codegen:"not_implemented,omit"`
    Comment        string   `mikrotik:"comment"         codegen:"comment"`
}

with further code generation:

$ go run ./cmd/mikrotik-codegen -src client/resource.go -struct MikrotikResource > mikrotik/resource_new.go

Next steps

As further improvement for this codebase, I see these useful features:

ddelnano commented 1 year ago

I tried out this PR against the client and terraform test changes in #153. I didn't get completely through that testing, but I added comments for the things I uncovered so far.

From performing that testing, it made me wonder if we could test that the output of go run ./cmd/mikrotik-codegen results code that compiles. The parser_test seems to already leverage the go parser and AST, so maybe that wouldn't be too difficult to do?

It would be very useful to be able to add unit tests for a give struct definition to ensure things like bool and int64 types use the correct ${TYPE}planmodifier.UseStateForUnknown(). The verification I did so far was time consuming, so being able to have an end to end test for that would be useful. Curious to hear your thoughts on this.

maksym-nazarenko commented 1 year ago

@ddelnano I addressed all the comments in the latest commits. however, I would like to emphasize, that current implementation is not intended to be fully automatic. I see at least these issues:

To fix types issues, I want to introduce some generic function that uses reflection, so it could be properly tested and just used in code generation.

To wrap-up: my intention is to ease the creation of new resources (by auto-generation boilerplate). Once we have enough use cases, we can proceed with enhancements.

ddelnano commented 1 year ago

Yep, completely understand that this is meant to ease the creation of the boilerplate. The current implementation lgtm. Sorry for the long turn around time on the latest review :ship: