-
Notifications
You must be signed in to change notification settings - Fork 17
feat: improved cost summaries and resource listing in account billing #632
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for the PR! Seems that this is at least partly duplicate of #606. We'll handle that one first as it was created earlier and we have already started review work there. Let's see after that how we can further improve the billing logs support. |
|
@kangasta Ideally the PR would include features both #606 and #632, since budget management is a major selling point for UpCloud over traditional cloud solutions. |
Implements 'upctl billing' commands to view billing information: ## billing summary View cost summary by category with flexible period options: - Named periods: 'month', 'day', 'quarter', 'year', 'last month' - Relative periods: '3months' (3 months ago), '2weeks' (2 weeks ago) - Relative from date: '2months from 2024-06', '+3months from 2024-01' - Direct format: 'YYYY-MM' - Optional --resource filter for specific resource UUID - Optional --detailed flag for full breakdown ## billing list List detailed billing with resource names: - Fetches and displays resource names (servers, storage) - Same flexible --period options as summary - --match flag for name filtering (case-insensitive) - --category flag to filter by resource type - Shows both UUID and name for identification Both commands: - Default to current month if period not specified - Support JSON/YAML output for scripting - Use existing GetBillingSummary API endpoint Addresses UpCloudLtd#339
- Add trailing newlines to files - Fix struct field alignment in resourceInfo - Fix string concatenation spacing
93c3b64 to
a5402bf
Compare
…fication - Add --period flag for intuitive date specification (month, last month, 3months, etc) - Add --detailed flag to show resource names alongside UUIDs - Add --match and --category flags for resource filtering - Default to current month when no period specified - Maintain full backward compatibility with --year/--month flags - Remove duplicate billing commands, enhance existing account billing instead Following reviewer guidance to avoid command duplication while preserving all valuable functionality from the original billing commands.
- Extract getCategories() and getResourceGroups() helper functions - Reuse helper functions across buildOriginalSections, buildEnhancedSections, and buildResourceRows - Add comprehensive backward compatibility tests - Verify year/month flags override period flag when both are specified - Reduce code duplication by ~45 lines while maintaining all functionality
|
The PR is now on top of
New Features Added to
|
kangasta
left a 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.
|
|
||
| // parsePeriod converts various period formats into YYYY-MM for the API | ||
| // Supports formats like: "month", "last month", "3months", "2024-07", "2months from 2024-06" | ||
| func parsePeriod(period string) (string, string, error) { |
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.
This argument/function seems quite complex and might cause confusion so maybe it could be omitted for now 🤔
We try to keep the user-interface quite simple and close to the API, so adding a non-standard way for defining the period would be quite different compared to options provided in other commands. I'm sorry that these design principles are not well documented in any public guidelines yet.
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 agree that was some work, but I would like to push for this feature as a first user: depending of the project one wants a quarterly, monthly, or biweekly billing.
| } | ||
|
|
||
| // fetchResourceNames retrieves names for servers and storage resources | ||
| func (s *billingCommand) fetchResourceNames(exec commands.Executor, summary *upcloud.BillingSummary) map[string]string { |
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.
We have some logic already for filtering resources by name in all list and all purge commands, maybe that could be re-used here 🤔 That code would not help finding resources that have already been deleted, though.
- Add concurrent fetching of resource names for better performance - Support more resource types: load balancers, databases, Kubernetes clusters - Use sync.WaitGroup for parallel API calls with thread-safe name collection - Gracefully handle errors to avoid breaking the entire billing display
- Extract common goroutine creation pattern into helper function - Use defer for mutex unlock to ensure proper cleanup - Reduce code duplication in resource fetching logic - Address golangci-lint suggestions for cleaner code
Improves 'upctl account billing' command to view billing information:
Account billing summary
Related to #339