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

Change actions billing structs to maps #2597

Merged
merged 2 commits into from Dec 26, 2022

Conversation

tragiclifestories
Copy link
Contributor

@tragiclifestories tragiclifestories commented Dec 13, 2022

Fixes #2595.

The structs hardcode the three machine types that Github Actions supported until recently. However, we now have a lot more different types of machines, with the potential for more to be added at any time. Using maps instead of structs will allow us to transparently support more machine classes in the future.

I've done this pretty naively instead of creating safe access methods or anything like that, and kept the map values as pointers for consistency with other maps in the codebase. Can't think of any tests that would be necessary (beyond the generated ones) but open to suggestions.

@gmlewis gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Dec 13, 2022
@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #2597 (c897aa1) into master (6216f64) will not change coverage.
The diff coverage is n/a.

❗ Current head c897aa1 differs from pull request most recent head f231951. Consider uploading reports for the commit f231951 to get more accurate results

@@           Coverage Diff           @@
##           master    #2597   +/-   ##
=======================================
  Coverage   97.99%   97.99%           
=======================================
  Files         126      126           
  Lines       10913    10913           
=======================================
  Hits        10694    10694           
  Misses        150      150           
  Partials       69       69           
Impacted Files Coverage Δ
github/actions_workflow_runs.go 100.00% <ø> (ø)
github/actions_workflows.go 100.00% <ø> (ø)
github/billing.go 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @tragiclifestories !

github/actions_workflow_runs_test.go Outdated Show resolved Hide resolved
James Turley added 2 commits December 14, 2022 16:23
Fixes google#2595.

The structs hardcode the three machine types that Github Actions
supported until recently. However, we now have a lot more different
types of machines, with the potential for more to be added at any time.
Using maps instead of structs will allow us to transparently support
more machine classes in the future.
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @tragiclifestories !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

Copy link
Contributor

@valbeat valbeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 26, 2022

Thank you, @valbeat !
Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new machine types to response of GetWorkflowRunUsageByID
3 participants