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

Update 8.5.6 #533

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions .build/actions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package main

import (
"context"
"flag"
"fmt"
"iter"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Specify the full import path for the 'iter' package

The import statement for "iter" lacks a full path. If iter is an external or local package, it should include the full path to ensure proper resolution.

Apply this diff to correct the import:

-import (
 	"context"
 	"flag"
 	"fmt"
-	"iter"
+	"github.com/yourusername/iter"
 	"log"
 	"time"

 	"github.com/google/go-github/v65/github"
 )

Replace "github.com/yourusername/iter" with the actual path to the iter package.

Committable suggestion skipped: line range outside the PR's diff.

"log"
"time"

"github.com/google/go-github/v65/github"
)

func newActions() actions {
return actions{}
}

func (this *actions) init(b *build, _ *flag.FlagSet) {
this.build = b
}
Comment on lines +18 to +20
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use idiomatic receiver names instead of 'this'

In Go, it's idiomatic to use a shorter, meaningful receiver name, often a single or double-letter abbreviation of the type name, rather than 'this'. For the actions type, consider using a *actions as the receiver name.

Apply this diff to rename the receiver:

-func (this *actions) init(b *build, _ *flag.FlagSet) {
-	this.build = b
+func (a *actions) init(b *build, _ *flag.FlagSet) {
+	a.build = b
 }

-func (this *actions) Validate() error { return nil }
+func (a *actions) Validate() error { return nil }

-func (this *actions) workflowByFilename(ctx context.Context, fn string) (*workflow, error) {
-	v, _, err := this.build.client.Actions.GetWorkflowByFileName(ctx, this.build.owner.String(), this.build.name.String(), fn)
+func (a *actions) workflowByFilename(ctx context.Context, fn string) (*workflow, error) {
+	v, _, err := a.build.client.Actions.GetWorkflowByFileName(ctx, a.build.owner.String(), a.build.name.String(), fn)
 	if err != nil {
-		return nil, fmt.Errorf("cannot retrieve workflow %s from %v: %w", fn, this.build, err)
+		return nil, fmt.Errorf("cannot retrieve workflow %s from %v: %w", fn, a.build, err)
 	}
 	return &workflow{
 		v,
-		this,
+		a,
 	}, nil
 }

Also applies to: 26-37


type actions struct {
build *build
}

func (this *actions) Validate() error { return nil }

func (this *actions) workflowByFilename(ctx context.Context, fn string) (*workflow, error) {
v, _, err := this.build.client.Actions.GetWorkflowByFileName(ctx, this.build.owner.String(), this.build.name.String(), fn)
if err != nil {
return nil, fmt.Errorf("cannot retrieve workflow %s from %v: %w", fn, this.build, err)
}
return &workflow{
v,
this,
}, nil
}

type workflow struct {
*github.Workflow

parent *actions
}

func (this *workflow) String() string {
return fmt.Sprintf("%s(%d)@%v", *this.Name, *this.ID, this.parent.build.repo)
}
Comment on lines +45 to +47
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use idiomatic receiver names for 'workflow' methods

For the workflow type, replace the receiver name this with a shorter name like w *workflow to follow Go conventions.

Apply this diff to rename the receiver:

-func (this *workflow) String() string {
-	return fmt.Sprintf("%s(%d)@%v", *this.Name, *this.ID, this.parent.build.repo)
+func (w *workflow) String() string {
+	return fmt.Sprintf("%s(%d)@%v", *w.Name, *w.ID, w.parent.build.repo)
 }

-func (this *workflow) runs(ctx context.Context) iter.Seq2[*workflowRun, error] {
+func (w *workflow) runs(ctx context.Context) iter.Seq2[*workflowRun, error] {
 	return func(yield func(*workflowRun, error) bool) {
 		var opts github.ListWorkflowRunsOptions
 		opts.PerPage = 100

 		for {
-			candidates, rsp, err := this.parent.build.client.Actions.ListWorkflowRunsByID(ctx, this.parent.build.owner.String(), this.parent.build.name.String(), *this.ID, &opts)
+			candidates, rsp, err := w.parent.build.client.Actions.ListWorkflowRunsByID(ctx, w.parent.build.owner.String(), w.parent.build.name.String(), *w.ID, &opts)
 			if err != nil {
-				yield(nil, fmt.Errorf("cannot retrieve workflow runs of %v (page: %d): %w", this, opts.Page, err))
+				yield(nil, fmt.Errorf("cannot retrieve workflow runs of %v (page: %d): %w", w, opts.Page, err))
 				return
 			}
 			for _, v := range candidates.WorkflowRuns {
-				if !yield(&workflowRun{v, this}, nil) {
+				if !yield(&workflowRun{v, w}, nil) {
 					return
 				}
 			}
 			if rsp.NextPage == 0 {
 				return
 			}
 			opts.Page = rsp.NextPage
 		}
 	}
 }

Also applies to: 49-71


func (this *workflow) runs(ctx context.Context) iter.Seq2[*workflowRun, error] {
return func(yield func(*workflowRun, error) bool) {
var opts github.ListWorkflowRunsOptions
opts.PerPage = 100

for {
candidates, rsp, err := this.parent.build.client.Actions.ListWorkflowRunsByID(ctx, this.parent.build.owner.String(), this.parent.build.name.String(), *this.ID, &opts)
if err != nil {
yield(nil, fmt.Errorf("cannot retrieve workflow runs of %v (page: %d): %w", this, opts.Page, err))
return
}
for _, v := range candidates.WorkflowRuns {
if !yield(&workflowRun{v, this}, nil) {
return
}
}
if rsp.NextPage == 0 {
return
}
opts.Page = rsp.NextPage
}
}
}

type workflowRun struct {
*github.WorkflowRun
parent *workflow
}

func (this *workflowRun) reload(ctx context.Context) error {
v, _, err := this.parent.parent.build.client.Actions.GetWorkflowRunByID(ctx, this.parent.parent.build.owner.String(), this.parent.parent.build.name.String(), *this.ID)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify access to deeply nested fields

Accessing fields like this.parent.parent.build can make the code hard to read and maintain. Consider storing a direct reference to build within the workflowRun type to improve readability.

Modify the workflowRun struct to include a direct reference to build:

 type workflowRun struct {
 	*github.WorkflowRun
 	parent *workflow
+	build  *build
 }

Update the constructor and methods accordingly:

-	if !yield(&workflowRun{v, w}, nil) {
+	if !yield(&workflowRun{v, w, w.parent.build}, nil) {
 		return
 	}

-func (wr *workflowRun) reload(ctx context.Context) error {
-	v, _, err := wr.parent.parent.build.client.Actions.GetWorkflowRunByID(ctx, wr.parent.parent.build.owner.String(), wr.parent.parent.build.name.String(), *wr.ID)
+	v, _, err := wr.build.client.Actions.GetWorkflowRunByID(ctx, wr.build.owner.String(), wr.build.name.String(), *wr.ID)

And similarly in other methods.

Also applies to: 97-97, 103-103

if err != nil {
return fmt.Errorf("cannot reload workflow run %v: %w", this, err)
}
this.WorkflowRun = v
return nil
}
Comment on lines +78 to +85
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use idiomatic receiver names for 'workflowRun' methods

For the workflowRun type, consider changing the receiver name from this to wr *workflowRun for clarity and to follow Go naming conventions.

Apply this diff to rename the receiver:

-func (this *workflowRun) reload(ctx context.Context) error {
+func (wr *workflowRun) reload(ctx context.Context) error {
-	v, _, err := this.parent.parent.build.client.Actions.GetWorkflowRunByID(ctx, this.parent.parent.build.owner.String(), this.parent.parent.build.name.String(), *this.ID)
+	v, _, err := wr.parent.parent.build.client.Actions.GetWorkflowRunByID(ctx, wr.parent.parent.build.owner.String(), wr.parent.parent.build.name.String(), *wr.ID)
 	if err != nil {
-		return fmt.Errorf("cannot reload workflow run %v: %w", this, err)
+		return fmt.Errorf("cannot reload workflow run %v: %w", wr, err)
 	}
-	this.WorkflowRun = v
+	wr.WorkflowRun = v
 	return nil
 }

-func (this *workflowRun) wait(ctx context.Context) error {
+func (wr *workflowRun) wait(ctx context.Context) error {
 	start := time.Now()
 	for ctx.Err() == nil {
-		if err := this.reload(ctx); err != nil {
+		if err := wr.reload(ctx); err != nil {
 			return err
 		}
-		if this.Status != nil && *this.Status == "completed" {
+		if wr.Status != nil && *wr.Status == "completed" {
 			return nil
 		}
-		log.Printf("INFO %v is still running (waiting since %v)...", this, time.Since(start).Truncate(time.Second))
-		time.Sleep(this.parent.parent.build.waitTimeout)
+		log.Printf("INFO %v is still running (waiting since %v)...", wr, time.Since(start).Truncate(time.Second))
+		time.Sleep(wr.parent.parent.build.waitTimeout)
 	}
 	return ctx.Err()
 }

-func (this *workflowRun) rerun(ctx context.Context) error {
+func (wr *workflowRun) rerun(ctx context.Context) error {
-	_, err := this.parent.parent.build.client.Actions.RerunWorkflowByID(ctx, this.parent.parent.build.owner.String(), this.parent.parent.build.name.String(), *this.ID)
+	_, err := wr.parent.parent.build.client.Actions.RerunWorkflowByID(ctx, wr.parent.parent.build.owner.String(), wr.parent.parent.build.name.String(), *wr.ID)
 	if err != nil {
-		return fmt.Errorf("cannot rerun workflow run %v: %w", this, err)
+		return fmt.Errorf("cannot rerun workflow run %v: %w", wr, err)
 	}
 	return nil
 }

Also applies to: 87-100, 102-108


func (this *workflowRun) wait(ctx context.Context) error {
start := time.Now()
for ctx.Err() == nil {
if err := this.reload(ctx); err != nil {
return err
}
if this.Status != nil && *this.Status == "completed" {
return nil
}
log.Printf("INFO %v is still running (waiting since %v)...", this, time.Since(start).Truncate(time.Second))
time.Sleep(this.parent.parent.build.waitTimeout)
}
return ctx.Err()
}

func (this *workflowRun) rerun(ctx context.Context) error {
_, err := this.parent.parent.build.client.Actions.RerunWorkflowByID(ctx, this.parent.parent.build.owner.String(), this.parent.parent.build.name.String(), *this.ID)
if err != nil {
return fmt.Errorf("cannot rerun workflow run %v: %w", this, err)
}
return nil
}

func (this workflowRun) String() string {
return fmt.Sprintf("%d@%v", *this.ID, this.parent)
}
Comment on lines +110 to +112
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure consistent receiver types in 'workflowRun' methods

The String method for workflowRun uses a value receiver, whereas other methods use a pointer receiver. For consistency and to prevent potential issues with copying, consider changing it to use a pointer receiver.

Apply this diff to change the receiver:

-func (this workflowRun) String() string {
-	return fmt.Sprintf("%d@%v", *this.ID, this.parent)
+func (wr *workflowRun) String() string {
+	return fmt.Sprintf("%d@%v", *wr.ID, wr.parent)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (this workflowRun) String() string {
return fmt.Sprintf("%d@%v", *this.ID, this.parent)
}
func (wr *workflowRun) String() string {
return fmt.Sprintf("%d@%v", *wr.ID, wr.parent)
}

73 changes: 73 additions & 0 deletions .build/build.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package main

import (
"context"
"flag"
"fmt"
"os"
"time"

"github.com/google/go-github/v65/github"
)

func newBuild() (build, error) {
r, err := newRepo()
if err != nil {
return build{}, err
}
return build{
repo: r,
waitTimeout: time.Second * 3,
commands: make(map[string]command),
}, nil
}

type build struct {
repo

waitTimeout time.Duration

client *github.Client

commands map[string]command
}

type command struct {
f func(context.Context, []string) error
usage string
}

func (this *build) init(fs *flag.FlagSet) {
this.client = github.NewClient(nil).
WithAuthToken(os.Getenv("GITHUB_TOKEN"))

fs.DurationVar(&this.waitTimeout, "wait-timeout", this.waitTimeout, "")

this.repo.init(this, fs)
}
Comment on lines +40 to +47
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add GitHub token validation.

The code doesn't validate if the GitHub token is present or valid. Consider:

  1. Checking if the token is empty
  2. Validating the token by making a test API call
  3. Returning an error if authentication fails

Here's a suggested improvement:

 func (this *build) init(fs *flag.FlagSet) {
+    token := os.Getenv("GITHUB_TOKEN")
+    if token == "" {
+        return fmt.Errorf("GITHUB_TOKEN environment variable is not set")
+    }
     this.client = github.NewClient(nil).
-        WithAuthToken(os.Getenv("GITHUB_TOKEN"))
+        WithAuthToken(token)
+
+    // Validate token by making a test API call
+    ctx := context.Background()
+    _, _, err := this.client.Users.Get(ctx, "")
+    if err != nil {
+        return fmt.Errorf("invalid GitHub token: %v", err)
+    }

Committable suggestion skipped: line range outside the PR's diff.


func (this *build) Validate() error {
if err := this.repo.Validate(); err != nil {
return err
}
return nil
}

func (this *build) registerCommand(name, usage string, action func(context.Context, []string) error) {
this.commands[name] = command{action, usage}
}
Comment on lines +56 to +58
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation for command registration.

The current implementation lacks validation for:

  1. Empty command name
  2. Nil action function
  3. Duplicate command names

Consider adding these checks to prevent runtime issues.

Here's a suggested improvement:

 func (this *build) registerCommand(name, usage string, action func(context.Context, []string) error) {
+    if name == "" {
+        panic("command name cannot be empty")
+    }
+    if action == nil {
+        panic("command action cannot be nil")
+    }
+    if _, exists := this.commands[name]; exists {
+        panic(fmt.Sprintf("command %q already registered", name))
+    }
     this.commands[name] = command{action, usage}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (this *build) registerCommand(name, usage string, action func(context.Context, []string) error) {
this.commands[name] = command{action, usage}
}
func (this *build) registerCommand(name, usage string, action func(context.Context, []string) error) {
if name == "" {
panic("command name cannot be empty")
}
if action == nil {
panic("command action cannot be nil")
}
if _, exists := this.commands[name]; exists {
panic(fmt.Sprintf("command %q already registered", name))
}
this.commands[name] = command{action, usage}
}


func (this *build) flagUsage(fs *flag.FlagSet, reasonMsg string, args ...any) {
w := fs.Output()
_, _ = fmt.Fprint(w, `Usage of .build:
`)
if reasonMsg != "" {
_, _ = fmt.Fprintf(w, "Error: %s\n", fmt.Sprintf(reasonMsg, args...))
}
_, _ = fmt.Fprintf(w, "Syntax: %s [flags] <command> [commandSpecificArgs]\nCommands:\n", fs.Name())
for n, c := range this.commands {
_, _ = fmt.Fprintf(w, "\t%s %s\n", n, c.usage)
}
_, _ = fmt.Fprint(w, "Flags:\n")
fs.PrintDefaults()
}
7 changes: 7 additions & 0 deletions .build/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module github.com/blaubaer/ha-addon/unifi/.build

go 1.23.1

require github.com/google/go-github/v65 v65.0.0

require github.com/google/go-querystring v1.1.0 // indirect
8 changes: 8 additions & 0 deletions .build/go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
github.com/google/go-github/v65 v65.0.0 h1:pQ7BmO3DZivvFk92geC0jB0q2m3gyn8vnYPgV7GSLhQ=
github.com/google/go-github/v65 v65.0.0/go.mod h1:DvrqWo5hvsdhJvHd4WyVF9ttANN3BniqjP8uTFMNb60=
github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8=
github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
93 changes: 93 additions & 0 deletions .build/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package main

import (
"context"
"errors"
"flag"
"fmt"
"os"
"os/signal"
"strings"
"syscall"
)

func main() {
b, err := newBuild()
if err != nil {
_, _ = fmt.Fprintf(os.Stderr, "Error: %v\n", err)
os.Exit(3)
}

fs := flag.NewFlagSet(os.Args[0], flag.ExitOnError)
fs.Usage = func() { b.flagUsage(fs, "") }
b.init(fs)

_ = fs.Parse(os.Args[1:])

if err := b.Validate(); err != nil {
b.flagUsage(fs, "ERROR: %v", err)
os.Exit(3)
}

err = func() error {
args := fs.Args()
if len(args) < 1 {
return flagFail("command missing")
}

cmd, ok := b.commands[args[0]]
if !ok {
return flagFail("unknown command: %s", args[0])
}

ctx, cancelFunc := context.WithCancel(context.Background())
sigs := make(chan os.Signal, 1)
defer close(sigs)
signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
go func() {
<-sigs
cancelFunc()
}()

Comment on lines +43 to +51
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve signal handling cleanup.

The current signal handling implementation could be enhanced:

  1. Signal notification should be stopped before program exit
  2. The goroutine might leak if the program exits before receiving a signal
 		ctx, cancelFunc := context.WithCancel(context.Background())
 		sigs := make(chan os.Signal, 1)
-		defer close(sigs)
+		defer func() {
+			signal.Stop(sigs)
+			close(sigs)
+			cancelFunc()
+		}()
 		signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
 		go func() {
-			<-sigs
-			cancelFunc()
+			select {
+			case <-sigs:
+				cancelFunc()
+			case <-ctx.Done():
+				return
+			}
 		}()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ctx, cancelFunc := context.WithCancel(context.Background())
sigs := make(chan os.Signal, 1)
defer close(sigs)
signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
go func() {
<-sigs
cancelFunc()
}()
ctx, cancelFunc := context.WithCancel(context.Background())
sigs := make(chan os.Signal, 1)
defer func() {
signal.Stop(sigs)
close(sigs)
cancelFunc()
}()
signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
go func() {
select {
case <-sigs:
cancelFunc()
case <-ctx.Done():
return
}
}()

return cmd.f(ctx, args[1:])
}()

var fe *flagError
if errors.As(err, &fe) {
b.flagUsage(fs, "ERROR: %v", err)
os.Exit(2)
} else if err != nil {
w := flag.CommandLine.Output()
_, _ = fmt.Fprintf(w, "%s: ERROR %v\n", os.Args[0], err)
os.Exit(3)
}
}

func flagFail(msg string, args ...any) *flagError {
err := flagError(fmt.Sprintf(msg, args...))
return &err
}

type stringSlice []string

func (this stringSlice) String() string {
return strings.Join(this, ",")
}

func (this *stringSlice) Set(s string) error {
var buf stringSlice
for _, plain := range strings.Split(s, ",") {
plain = strings.TrimSpace(plain)
if plain != "" {
buf = append(buf, plain)
}
}
*this = buf
return nil
}
Comment on lines +77 to +87
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding safety checks and optimizing performance.

The string slice handling could be improved with:

  1. Input validation to prevent potential memory issues
  2. Pre-allocation of the slice for better performance
 func (this *stringSlice) Set(s string) error {
+	const maxItems = 1000 // Adjust based on your needs
 	var buf stringSlice
+	parts := strings.Split(s, ",")
+	if len(parts) > maxItems {
+		return fmt.Errorf("too many items: %d (max %d)", len(parts), maxItems)
+	}
+	buf = make(stringSlice, 0, len(parts))
-	for _, plain := range strings.Split(s, ",") {
+	for _, plain := range parts {
 		plain = strings.TrimSpace(plain)
 		if plain != "" {
 			buf = append(buf, plain)

Committable suggestion skipped: line range outside the PR's diff.


type flagError string

func (this flagError) Error() string {
return string(this)
}
Comment on lines +89 to +93
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling capabilities.

Consider implementing the Unwrap interface and adding more context to errors:

 type flagError string
 
 func (this flagError) Error() string {
 	return string(this)
 }
+
+// Unwrap returns the underlying error if this error wraps another error
+func (this flagError) Unwrap() error {
+	return fmt.Errorf("flag error: %s", string(this))
+}

Committable suggestion skipped: line range outside the PR's diff.

Loading