Feat/sakura #1
Feat/sakura #1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates DevOpsBot from ConoHa VPS API to Sakura VPS API. The implementation includes updating the module name, replacing the authentication mechanism from token-based to Bearer token authentication, and adapting all API endpoints and response structures to match the Sakura VPS API specification.
Changes:
- Renamed module from
github.com/traPtitech/DevOpsBottogithub.com/traPtitech/sakura-DevOpsBot - Replaced ConoHa API configuration and authentication with Sakura VPS Bearer token authentication
- Updated server operations (restart, hosts) to use Sakura VPS API endpoints and response structures
- Removed unused dependencies (golang.org/x/sync) and simplified the restart command to remove the soft/hard restart type distinction
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updated module name and removed golang.org/x/sync dependency |
| go.sum | Updated checksums for dependency changes |
| main.go | Updated import path to use sakura-DevOpsBot module name |
| Dockerfile | Updated build command to reference new module name |
| cmd/root.go | Updated import paths |
| cmd/server.go | Updated import paths |
| pkg/config/config.go | Replaced ConoHa configuration with Sakura VPS configuration including origin, API path, and bearer token |
| pkg/server/token.go | Commented out entire ConoHa token authentication implementation |
| pkg/server/server.go | Added Japanese comment for command registration |
| pkg/server/restart.go | Refactored to use Sakura VPS force-reboot endpoint, simplified to single server ID argument, removed soft/hard restart types |
| pkg/server/hosts.go | Completely rewritten to match Sakura VPS API response structure, removed parallel goroutine fetching, updated logging format |
| pkg/bot/bot.go | Updated import paths |
| pkg/bot/command.go | Updated import paths |
| pkg/bot/help.go | Updated import paths |
| pkg/bot/traq/bot.go | Updated import paths, reordered imports |
| pkg/bot/traq/context.go | Updated import paths, reordered imports |
| pkg/bot/slack/bot.go | Updated import paths, reordered imports |
| pkg/bot/slack/context.go | Updated import paths, reordered imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logStr := fmt.Sprintf(`Request | ||
| - URL: %s | ||
| - RestartType: %s | ||
| Response | ||
| - Header: %+v | ||
| - Body: %s | ||
| - Status: %s (Expected: 202) | ||
| `, req.URL.String(), restartType, resp.Header, string(respBody), resp.Status) | ||
| `, req.URL.String(), resp.Header, string(respBody), resp.Status) | ||
| log.Println(logStr) |
There was a problem hiding this comment.
Logging the full response body and headers may expose sensitive information. Consider reviewing what information is included in the response and whether it's safe to log it. If the response contains any authentication tokens, server credentials, or other sensitive data, this could be a security concern.
| Name string `json:"name"` | ||
| } `json:"servers"` | ||
| Results []serverResponse `json:"results"` | ||
| Count string `json:"count"` |
There was a problem hiding this comment.
The Count field is defined as string, but count fields in paginated API responses are typically integers. Verify that the Sakura VPS API actually returns count as a string. If it's a number, this should be changed to int or int64 to match the API specification and avoid potential parsing issues.
| Count string `json:"count"` | |
| Count int `json:"count"` |
| CPUCores string `json:"cpu_cores"` | ||
| MemoryMiB string `json:"memory_mebibytes"` | ||
| Storage []struct { |
There was a problem hiding this comment.
Fields like CPUCores, MemoryMiB, and SizeGiB are defined as strings, but these represent numeric values. While this might be intentional to match the API response format, it makes numeric operations difficult and could lead to confusion. Verify the actual API response format and consider using appropriate numeric types (int, int64) if the API returns numbers, or document why strings are used if it's a deliberate choice.
| CPUCores string `json:"cpu_cores"` | |
| MemoryMiB string `json:"memory_mebibytes"` | |
| Storage []struct { | |
| // NOTE: CPUCores, MemoryMiB, and SizeGiB are defined as strings because the upstream API | |
| // encodes these numeric values as JSON strings. Convert to numeric types at use sites | |
| // if arithmetic or comparisons are needed. | |
| CPUCores string `json:"cpu_cores"` | |
| MemoryMiB string `json:"memory_mebibytes"` | |
| Storage []struct { |
| CpuCores: server.CPUCores, | ||
| MemoryMiB: server.MemoryMiB, | ||
| } | ||
| if server.Ipv6.Address != nil { | ||
| serverData.Ipv6 = *server.Ipv6.Address | ||
| } else { | ||
| serverData.Ipv6 = "null" | ||
| } | ||
| for _, storage := range server.Storage { | ||
| serverData.Storage = append(serverData.Storage, struct { | ||
| Size string | ||
| Type string | ||
| }{storage.SizeGiB, storage.Type}) | ||
| } | ||
| servers = append(servers, serverData) | ||
| } | ||
|
|
||
| sort.Slice(servers, func(i, j int) bool { | ||
| return servers[i].Name < servers[j].Name | ||
| }) | ||
|
|
||
| log.Printf("Server Name :\t Server ID, \t Zone Name, \t ipv4 address, \t ipv6 address, \t Memory size, \t Storage size(Storage type)") | ||
| for _, server := range servers { | ||
| log.Printf("%s: %s\n", server.Name, server.Host) | ||
| logMsg := fmt.Sprintf("%s :\t %s, \t %s, \t %s, \t %s, \t %sMiB", server.Name, server.ID, server.Zone, server.Ipv4, server.Ipv6, server.MemoryMiB) |
There was a problem hiding this comment.
The resultData struct includes CpuCores field (line 75) and it's populated from the server response (line 121), but this information is never displayed in the log output. The log header on line 142 and the log message on line 144 do not include CPU cores. Either add CPU cores to the output or remove it from the resultData struct if it's not needed.
| type serversResponse struct { | ||
| Servers []struct { | ||
| ID string `json:"id"` | ||
| Links []struct { | ||
| Href string `json:"href"` | ||
| Rel string `json:"rel"` | ||
| } `json:"links"` | ||
| Name string `json:"name"` | ||
| } `json:"servers"` | ||
| Results []serverResponse `json:"results"` | ||
| Count string `json:"count"` | ||
| Next *string `json:"next,omitempty"` | ||
| Previous *string `json:"previous,omitempty"` | ||
| } |
There was a problem hiding this comment.
The serversResponse includes pagination fields (Next, Previous) but the code only fetches the first page of results. If there are more servers than fit in one page, they won't be displayed. Consider either implementing pagination to fetch all servers, or documenting that only the first page is shown.
| serverID, err := strconv.Atoi(args[0]) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get conoha api token: %w", err) | ||
| return fmt.Errorf("invalid arguments, server id is must be integer") |
There was a problem hiding this comment.
The error message contains a grammatical error. It should say "server id must be an integer" instead of "server id is must be integer".
| return fmt.Errorf("invalid arguments, server id is must be integer") | |
| return fmt.Errorf("invalid arguments, server id must be an integer") |
| Set("Accept", "application/json"). | ||
| Set("X-Auth-Token", token). | ||
| Base(config.C.Servers.Sakura.Origin). | ||
| Post(fmt.Sprintf("%s/%d/force-reboot", config.C.Servers.Sakura.ServersApiURLPath, serverID)). |
There was a problem hiding this comment.
The URL construction using fmt.Sprintf concatenates paths that may result in double slashes or incorrect paths. With the default ServersApiURLPath starting with "/" and the format string adding "/%d/force-reboot", you're correctly getting "/vps/api/v7/servers/123/force-reboot". However, this assumes ServersApiURLPath always starts with "/". Consider documenting this requirement or using path.Join to make the construction more robust.
| req, err := sling.New(). | ||
| Base(config.C.Servers.Conoha.Origin.Compute). | ||
| Get(fmt.Sprintf("v2/%s/servers", config.C.Servers.Conoha.TenantID)). | ||
| Set("Accept", "application/json"). | ||
| Set("X-Auth-Token", token). | ||
| Base(config.C.Servers.Sakura.Origin). | ||
| Get(config.C.Servers.Sakura.ServersApiURLPath). | ||
| Add("Authorization", "Bearer "+config.C.Servers.Sakura.BearerToken). |
There was a problem hiding this comment.
There's no validation to check if the BearerToken is empty before making API requests. If the token is not configured, the API call will fail with an authentication error. Consider adding validation to check if the Bearer token is configured and return a clearer error message early in the execution.
| req, err := sling.New(). | ||
| Base(config.C.Servers.Conoha.Origin.Compute). | ||
| Post(fmt.Sprintf("v2/%s/servers/%s/action", config.C.Servers.Conoha.TenantID, serverID)). | ||
| BodyJSON(m{"reboot": m{"type": args[0]}}). | ||
| Set("Accept", "application/json"). | ||
| Set("X-Auth-Token", token). | ||
| Base(config.C.Servers.Sakura.Origin). | ||
| Post(fmt.Sprintf("%s/%d/force-reboot", config.C.Servers.Sakura.ServersApiURLPath, serverID)). | ||
| Add("Authorization", "Bearer "+config.C.Servers.Sakura.BearerToken). |
There was a problem hiding this comment.
There's no validation to check if the BearerToken is empty before making API requests. If the token is not configured, the API call will fail with an authentication error. Consider adding validation to check if the Bearer token is configured and return a clearer error message early in the execution.
| return servers[i].Name < servers[j].Name | ||
| }) | ||
|
|
||
| log.Printf("Server Name :\t Server ID, \t Zone Name, \t ipv4 address, \t ipv6 address, \t Memory size, \t Storage size(Storage type)") |
There was a problem hiding this comment.
The log header line contains grammatical inconsistencies in the column labels. "Memory size" should more specifically be "Memory Size (MiB)" or similar, and "Storage size(Storage type)" has inconsistent spacing. Consider using consistent formatting like "Storage Size (GiB) / Type" or "Storage (Size GiB, Type)".
DevOpsBotをさくらVPSのAPIに対応させました。
CIがまだ整備できていません。
かつまだテストしてないのでDraftです。
このあとローカルテストします。