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

Add egbuilder #1058

Merged
merged 16 commits into from
Aug 15, 2023
Merged

Add egbuilder #1058

merged 16 commits into from
Aug 15, 2023

Conversation

suchen-sci
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.08% ⚠️

Comparison is base (c4bc656) 81.70% compared to head (3d82d1e) 81.63%.
Report is 2 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1058      +/-   ##
==========================================
- Coverage   81.70%   81.63%   -0.08%     
==========================================
  Files         135      135              
  Lines       15121    15138      +17     
==========================================
+ Hits        12355    12358       +3     
- Misses       2209     2225      +16     
+ Partials      557      555       -2     

see 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

doc/egbuilder.md Outdated
raceDetector: false

# skipBuild: if true, not build, just generate temp directory and files.
# still clean up if skipCleanUp is false.
Copy link
Collaborator

Choose a reason for hiding this comment

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

the meaning of this line is not clear enough, please rewrite it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all updated, please check!

doc/egbuilder.md Outdated
Comment on lines 121 to 122
egbuilder run // run with default
egbuilder run -f run.yaml // run with config
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are shell commands, // is not the correct comment format.

if !unicode.IsUpper(nameArr[0]) {
return false
}
return ValidVariableName(string(nameArr))
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not using ValidVariableName(name)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be ExportableVariableName for the function name... I will update the function name

return true
}

func CapitalVariableName(name string) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the name of this function is not clearly described its functionality.

"fmt"
"strings"

j "github.com/dave/jennifer/jen"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is fine, but I think using 'text/template' for this task is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jennifer can do some checks. And can split the generated files into pieces. Emmm, it's a trade off choice.

}
fmt.Println("generate controllers success")

err = initConfig.GenRegistry(cwd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

will all controllers and filters be added to registry even some of them failed to generate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if generation of file failed, the process will exit.

@localvar localvar enabled auto-merge August 4, 2023 01:12
@localvar localvar added this pull request to the merge queue Aug 15, 2023
Merged via the queue into easegress-io:main with commit be7e355 Aug 15, 2023
7 checks passed
@suchen-sci suchen-sci deleted the add-egbuilder branch August 15, 2023 08:17
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.

None yet

4 participants