-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Render service.go
for entry and config
#42
Conversation
…d functions used in templates
… the issues from `golangci-lint run --enable-all`
…hich from now is used also in config.tmpl
491b5e3
to
ec3c02e
Compare
templates/sdk/service.tmpl
Outdated
import ( | ||
"context" | ||
"fmt" | ||
|
||
"github.com/PaloAltoNetworks/pango/errors" | ||
"github.com/PaloAltoNetworks/pango/filtering" | ||
"github.com/PaloAltoNetworks/pango/util" | ||
"github.com/PaloAltoNetworks/pango/xmlapi" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't statically define imports, just use the importlib helper. Firstly because that's why it exists, and second because even pango will eventually need to import other libs (think subconfig). So let the importlib help with that.
./pkg/imports/manager.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used imports manager:
{{renderImports "service"}} |
Now that list is static for every template:
func RenderImports(templateType string) (string, error) { |
but when I start working with more advanced objects, which are using other objects - I need to import them, so I will extend that logic in other PRs.
package {{packageName .GoSdkPath}} | ||
{{- if .Entry}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entry style normalizations and Config style normalizations should probably be separate files.
This becomes more apparent now that we need to add support for UUID-Entry styled services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this moment I propose to leave it as it's - in next stages, when I start work on UUID, I can split it. If we will have other template for that login for configs, what should be the output file name , if service.go
is going to be reserved for entries ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still be service.go
because it's the service for that namespace. Just the contents of the service is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now from the name of the template , we are creating final file:
pan-os-codegen/pkg/generate/generator.go
Line 77 in 03809ff
func (c *Creator) createFullFilePath(templateName string) string { |
so from service.tmpl
we are creating service.go
.
If we want to split it to different templates, but receive file service.go
, then I need to rebuild that logic - I propose to do it in other PR, ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, then I wouldn't wrap the entirety of the file in an if/else, then. There are not a ton of changes between an Entry service.go and a Config service.go. Be more specific with the template instead of splitting it into separate template files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GJ! Please check my comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few things left.
package {{packageName .GoSdkPath}} | ||
{{- if .Entry}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still be service.go
because it's the service for that namespace. Just the contents of the service is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Description
PR delivers mainly new template used to render
service.go
for entry and configMotivation and Context
#32
How Has This Been Tested?
Template was checked locally by generating code from the template and trying to created, update and delete:
New function is tested via unit test.
Types of changes
Checklist