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

Code generation #150

Merged
merged 6 commits into from
May 24, 2023
Merged

Code generation #150

merged 6 commits into from
May 24, 2023

Conversation

maksym-nazarenko
Copy link
Collaborator

@maksym-nazarenko maksym-nazarenko commented May 6, 2023

Closes #47

This PR replaces #104.

Complete before merge:

  • [ ] errors typing (don't have a clear vision, so skipping atm)
  • use defined types to represent struct field types (currently, they are literal strings)

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:

  • Generate client API resource as well (based on struct definition only)
  • Allow generating different parts of code: .... -only={client,client_test,terraform_resource,terraform_test}

@maksym-nazarenko maksym-nazarenko requested a review from ddelnano May 7, 2023 10:06
Copy link
Owner

@ddelnano ddelnano left a comment

Choose a reason for hiding this comment

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

Thanks for revamping this and I'm very excited to have this capability soon! I haven't reviewed everything yet (inner part of the code generation to go still).

I should be able to get through the rest of it this week. I intend to go through workflow end to end as I review the rest of this as well. I think I need to see it in action in order to effectively review the more involved parts of this change.

go.mod Show resolved Hide resolved
cmd/mikrotik-codegen/main.go Outdated Show resolved Hide resolved
cmd/mikrotik-codegen/main.go Outdated Show resolved Hide resolved
cmd/mikrotik-codegen/main.go Show resolved Hide resolved
cmd/mikrotik-codegen/internal/codegen/parser.go Outdated Show resolved Hide resolved
cmd/mikrotik-codegen/internal/codegen/parser.go Outdated Show resolved Hide resolved
mikrotik/resource_bridge_test.go Outdated Show resolved Hide resolved
@ddelnano
Copy link
Owner

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.

fix resource name case, use Field's type name for UseStateForUnknown()

fix func name in template
@maksym-nazarenko
Copy link
Collaborator Author

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

  • it is not possible to use custom imports (e.g. if resource has no Computed field, UseStateForUnknown() is not used, so the import also should be skipped). I have something in my head, but I don't think I can implement it right away due to lack of time.
  • For int field, the generator should wrap it in int64() and vice-versa for modelTo...() and ...ToModel() funcs.
  • Generator does not support custom types like MikrotikDuration

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

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 🚢

@ddelnano ddelnano merged commit a8fcc6e into ddelnano:master May 24, 2023
@ddelnano ddelnano added the enhancement New feature or request label Jun 6, 2023
@maksym-nazarenko maksym-nazarenko deleted the codegen branch September 19, 2023 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement mikrotik resources with code generation
2 participants