changed the html string in the hscontrol/handlers.go file using go-elem #2120
Conversation
WalkthroughThis pull request introduces a new dependency on the Changes
Possibly related PRs
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (6)
Files not reviewed due to no reviewable changes (2)
Additional comments not posted (12)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (2)
- go.mod (1 hunks)
- hscontrol/handlers.go (2 hunks)
Additional comments not posted (6)
hscontrol/handlers.go (5)
13-15: LGTM!The new imports for the
elem-golibrary and its sub-packages are valid and necessary for the changes made in this file.
144-149: LGTM!The new variable
codeStyleRegisterWebAPIis correctly defined and the styles are valid.
151-173: LGTM!The new variable
htmlRegisterWebAPIis correctly defined and the HTML structure is valid.The HTML structure has been modularized using the
elem-golibrary, which improves the maintainability and readability of the code by separating the HTML structure from the logic. This is a great improvement over the previous inline HTML approach.
176-176: LGTM!The change to
registerWebAPITemplateto referencehtmlRegisterWebAPI.Render()is valid and consistent with the new approach of using theelem-golibrary for rendering HTML.
Line range hint
184-248: Skipped review.The
RegisterWebAPIfunction has not been modified in this diff, so no review comments are necessary.go.mod (1)
82-82: LGTM!The new indirect dependency
github.com/chasefleming/elem-goat versionv0.28.0is consistent with the changes made inhscontrol/handlers.goto use theelem-golibrary for rendering HTML elements.
hscontrol/handlers.go
Outdated
| elem.H2(nil, elem.Text("Machine registration")), | ||
| elem.P(nil, elem.Text("Run the command below in the headscale server to add this machine to your network:")), | ||
| elem.Code(attrs.Props{attrs.Style: codeStyleRegisterWebAPI.ToInline()}, | ||
| elem.Raw(`headscale nodes register --user USERNAME --key {{.Key}}`)), |
There was a problem hiding this comment.
Lets drop the use of the templates, this is the only thing needing it, so you can make this whole thing a function, and then pass a key string argument, which is then used here
I think it would be something like:
| elem.Raw(`headscale nodes register --user USERNAME --key {{.Key}}`)), | |
| elem.Text(fmt.Sprintf("headscale nodes register --user USERNAME --key %s", key)), |
hscontrol/handlers.go
Outdated
| elem.Code(attrs.Props{attrs.Style: codeStyleRegisterWebAPI.ToInline()}, | ||
| elem.Raw(`headscale nodes register --user USERNAME --key {{.Key}}`)), | ||
| ))) | ||
|
|
||
| var registerWebAPITemplate = template.Must( |
There was a problem hiding this comment.
then this can be removed and the places that calls it will call the function you create above with the render method.
There was a problem hiding this comment.
I made it a function that returns a *elem.Element type and the caller calls Render() on it. I `defined the function. I didn't add error for the return I didn't find any place that needed it except if check for the config.Key is needed. Should I
here is the where the function is called
hscontrol/handlers.go
Outdated
| elem.Head( | ||
| nil, | ||
| elem.Title( | ||
| func registerWebAPITemplate(config registerWebAPITemplateConfig) *elem.Element { |
There was a problem hiding this comment.
you can drop the whole config and just make this a string
There was a problem hiding this comment.
then the config struct can be deleted.
hscontrol/handlers.go
Outdated
|
|
||
| var registerWebAPITemplate = template.Must( | ||
| template.New("registerweb").Parse(htmlRegisterWebAPI.Render())) | ||
| elem.Text(fmt.Sprintf("headscale nodes register --user USERNAME --key %s", config.Key)), |
hscontrol/handlers.go
Outdated
| elem.Head( | ||
| nil, | ||
| elem.Title( | ||
| func registerWebAPITemplate(config registerWebAPITemplateConfig) *elem.Element { |
There was a problem hiding this comment.
This can be renamed to something like registerWebHTML
hscontrol/handlers.go
Outdated
| @@ -210,34 +206,17 @@ func (h *Headscale) RegisterWebAPI( | |||
| return | |||
| } | |||
|
|
|||
| var content bytes.Buffer | |||
| if err := registerWebAPITemplate.Execute(&content, registerWebAPITemplateConfig{ | |||
| var htmlContent = registerWebAPITemplate(registerWebAPITemplateConfig{ | |||
hscontrol/handlers.go
Outdated
|
|
||
| writer.Header().Set("Content-Type", "text/html; charset=utf-8") | ||
| writer.WriteHeader(http.StatusOK) | ||
| _, err = writer.Write(content.Bytes()) | ||
| if err != nil { | ||
| if _, err := writer.Write([]byte(htmlContent.Render())); err != nil { |
There was a problem hiding this comment.
and then you can call it here like:
if _, err := writer.Write([]byte(registerWebHTML(machineKey.String()).Render())); err != nil {
|
I have implemented the suggestions and have also replaced the windows.html and apple.html files with windows.go and apple.go files. |
| @@ -134,38 +135,37 @@ func (h *Headscale) HealthHandler( | |||
| respond(nil) | |||
| } | |||
|
|
|||
| type registerWebAPITemplateConfig struct { | |||
| Key string | |||
| var codeStyleRegisterWebAPI = styles.Props{ | |||
There was a problem hiding this comment.
Lets move this to templates/, making as much as possible reusable
| </body> | ||
| </html> | ||
| `)) | ||
| func registerWebHTML(key string) *elem.Element { |
There was a problem hiding this comment.
Lets make a templates package, templates, rename it to something like RegisterWeb and then we can make more of the generic HTML reusable.
| `)) | ||
| func registerWebHTML(key string) *elem.Element { | ||
| return elem.Html(nil, | ||
| elem.Head( |
There was a problem hiding this comment.
elem.Html,elem.Head, elem.Body can be made reusable between RegisterWeb, Windows and Apple.
hscontrol/templates/apple.go
Outdated
| "github.com/chasefleming/elem-go/attrs" | ||
| ) | ||
|
|
||
| func HeadscaleApple(url string) *elem.Element { |
There was a problem hiding this comment.
This can just be called Apple
hscontrol/templates/apple.go
Outdated
| elem.Text("headscale - Apple")), | ||
| elem.Style(nil, | ||
| elem.CSS(` | ||
| body { |
There was a problem hiding this comment.
Lets make this use the elem-go style syntax and reuse it between Windows and Apple.
There was a problem hiding this comment.
I thought if I do that, It will be redundant for some elements, like the style props should be passed to each header element. Let me do it the way you suggested and I'll let you know
hscontrol/templates/apple.go
Outdated
|
|
||
| func HeadscaleApple(url string) *elem.Element { | ||
| return elem.Html(nil, | ||
| elem.Head(attrs.Props{ |
There was a problem hiding this comment.
This whole section could be made generic and reused.
hscontrol/templates/apple.go
Outdated
| elem.Li(nil, | ||
| elem.Text("ALT + Click the Tailscale icon in the menu and hover over the Debug menu")), | ||
| elem.Li(nil, | ||
| elem.Text("Under \"Custom Login Server\", select \"Add Account...\"")), |
There was a problem hiding this comment.
When having a string in go that uses ", you should do:
| elem.Text("Under \"Custom Login Server\", select \"Add Account...\"")), | |
| elem.Text(`Under "Custom Login Server", select "Add Account..."`)), |
to not have to escape everything.
|
@amha-mersha How is this going? sorry for hammering you with comments :) |
|
Hello @kradalby , sorry I had some other work and forgot about it. I will correct them today and push another commit. Thank you for the constant feedbacks and the follow up, It really helps me learn software production |
|
@kradalby hopefully I have made it more DRY now. |
There was a problem hiding this comment.
Only a small nit, I think it looks good now. Could you try to rebase this so it does not contain all the other commits since you started?
I would like to have @nblock have a quick look over the html pages so we are sure we didnt loose any info.
hscontrol/templates/apple.go
Outdated
| elem.Title(nil, | ||
| elem.Text("headscale - Apple")), | ||
| elem.Body(attrs.Props{ | ||
| attrs.Style: bodySyle.ToInline(), |
There was a problem hiding this comment.
Typo
| attrs.Style: bodySyle.ToInline(), | |
| attrs.Style: bodyStyle.ToInline(), |
Created templates package in ./hscontrol/templates. Moved the registerWebAPITemplate into the templates package as a function to be called. Replaced the apple and windows html files with go-elem.
amha-mersha
force-pushed
the
amha.refactoring_html_codes_using_goelem
branch
from
3581c81 to
6972b0b
Compare
Summary by CodeRabbit
New Features
elem-golibrary, allowing for easier updates to the registration page.Bug Fixes