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

Share common types between Autoscaler versions #1663

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vboulineau
Copy link
Contributor

What does this PR do?

Share common types between Autoscaler versions to avoid useless conversions.

Motivation

Additional Notes

Minimum Agent Versions

Describe your test plan

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label

@vboulineau vboulineau added enhancement New feature or request qa/skip-qa labels Feb 6, 2025
@vboulineau vboulineau requested a review from a team as a code owner February 6, 2025 13:16
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.49%. Comparing base (b8dad05) to head (fa71b40).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1663   +/-   ##
=======================================
  Coverage   49.49%   49.49%           
=======================================
  Files         218      218           
  Lines       21244    21244           
=======================================
  Hits        10515    10515           
  Misses      10182    10182           
  Partials      547      547           
Flag Coverage Δ
unittests 49.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8dad05...fa71b40. Read the comment docs.

@vboulineau vboulineau force-pushed the vboulineau/dpa-common branch 3 times, most recently from 1f18c4b to bcee78a Compare February 7, 2025 08:54
@vboulineau vboulineau requested a review from a team as a code owner February 7, 2025 08:54
@vboulineau vboulineau force-pushed the vboulineau/dpa-common branch 3 times, most recently from ceeeba6 to 1c6451e Compare February 7, 2025 12:39
@vboulineau vboulineau force-pushed the vboulineau/dpa-common branch from 1c6451e to 380fff1 Compare February 10, 2025 14:06
@celenechang celenechang added this to the v1.13.0 milestone Feb 10, 2025
Copy link
Member

@tbavelier tbavelier left a comment

Choose a reason for hiding this comment

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

just a nit on possibly defining solely uint64pointer while relying on other existing utils

Comment on lines +16 to +20
// NewPointer returns a pointer to a new value instance
func NewPointer[T any](v T) *T {
return &v
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// NewPointer returns a pointer to a new value instance
func NewPointer[T any](v T) *T {
return &v
}
// NewUInt64Pointer returns pointer on a new uint64 value instance
func NewUInt64Pointer(i uint64) *uint64 {
return &i
}

This NewPointer is solely used for uint64 as a new "case", while all other uses can be replaced with existing NewInt32Pointer, NewBoolPointer, but no strong opinion

APIVersion: "apps/v1",
},
Owner: "test-owner",
RemoteVersion: utils.NewPointer[uint64](10),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RemoteVersion: utils.NewPointer[uint64](10),
RemoteVersion: utils.NewUInt64Pointer(10),

See comment on utils.go file

Name: v1.ResourceCPU,
Value: common.DatadogPodAutoscalerTargetValue{
Type: common.DatadogPodAutoscalerUtilizationTargetValueType,
Utilization: utils.NewPointer[int32](80),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Utilization: utils.NewPointer[int32](80),
Utilization: utils.NewInt32Pointer(80),

Ditto

return nil
}

// As many type are shared, we'll assign the deep copied value to the new object
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// As many type are shared, we'll assign the deep copied value to the new object
// As many types are shared, we'll assign the deep copied value to the new object

return nil
}

// As many type are shared, we'll assign the deep copied value to the new object
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// As many type are shared, we'll assign the deep copied value to the new object
// As many types are shared, we'll assign the deep copied value to the new object

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request qa/skip-qa
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants