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

Make azure and async spec type default values consistent with Azure Go SDK types #2494

Closed
Jont828 opened this issue Jul 21, 2022 · 11 comments
Closed
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@Jont828
Copy link
Contributor

Jont828 commented Jul 21, 2022

/kind bug

The azure.AgentPoolSpec type (found in azure/types.go](https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/types.go)) has fields such as AgentPoolSpec.AvailabilityZones which is a []string and AgentPoolSpec.VnetSubnetID which is a string. These types are used to initialize the Go SDK type containerservice.AgentPool which has corresponding pointer types such as AgentPool.AvailabilityZones which is a *[]string and AgentPool.VnetSubnetID which is a *string with code like this.

containerservice.AgentPool{
    AvailabilityZones: &spec.AvailabilityZones,
    ...
    VnetSubnetID:      &spec.VnetSubnetID,
}

As a result, when spec.AvailabilityZones and spec.VnetSubnetID are uninitialized with values [] and "", AgentPool.AvailabilityZones is set to &nil instead of nil and AgentPool.VnetSubnetID is set to &"" instead of nil. This results in a bug where the Azure Go SDK fields become initialized with pointers to default values. If the spec field is guaranteed to be set, this is not noticeable, but if not it will cause unit tests for the spec to fail.

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 21, 2022
@Jont828
Copy link
Contributor Author

Jont828 commented Jul 21, 2022

@CecileRobertMichon

@Jont828
Copy link
Contributor Author

Jont828 commented Jul 21, 2022

Another consideration is that sometimes we might want to pass a pointer to an int type. For example, containerservice.AgentPool.OSDiskSizeGB has the following description:

field OsDiskSizeGB *int32
OsDiskSizeGB - OS Disk Size in GB to be used to specify the disk size for every machine in this master/agent pool. If you specify 0, it will apply the default osDisk size according to the vmSize specified.

In this case we might not want to have it be nil if OS disk size is not in the spec.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 19, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 18, 2022
@nawazkh
Copy link
Member

nawazkh commented Nov 18, 2022

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 18, 2022
@dtzar
Copy link
Contributor

dtzar commented Jan 4, 2023

IMO we should prioritize the migration to a supported SDK track first and fix this issue as we go along (if it's still appropriate).

@willie-yao
Copy link
Contributor

/lifecycle frozen

We are prioritizing migration to ASO and will re-evaluate this depending on if it's still needed. More details at #3113

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Mar 3, 2023
@sonasingh46 sonasingh46 added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Aug 3, 2023
@dtzar dtzar added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed priority/backlog Higher priority than priority/awaiting-more-evidence. labels Aug 3, 2023
@sonasingh46 sonasingh46 removed the kind/bug Categorizes issue or PR as related to a bug. label Aug 3, 2023
@dtzar dtzar added kind/bug Categorizes issue or PR as related to a bug. and removed kind/bug Categorizes issue or PR as related to a bug. labels Aug 3, 2023
@dtzar
Copy link
Contributor

dtzar commented Dec 13, 2023

Do you think this is still needed @Jont828 @nojnhuh @mboersma?

@dtzar dtzar added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 13, 2023
@nojnhuh
Copy link
Contributor

nojnhuh commented Dec 13, 2023

I think the kinds of bugs this attempts to address are much less of a concern now with ASO. Overall I'm +1 to closing this and re-opening if we do encounter any more bugs that this would have saved. Or at least recording that this is very low priority.

@Jont828
Copy link
Contributor Author

Jont828 commented Dec 14, 2023

I'll defer to @nojnhuh on this since most of these spec types were replaced with the ASO types. Closing it out for now, and if we need to reopen, we could probably just make a new issue specific to ASO.

/close

@k8s-ci-robot
Copy link
Contributor

@Jont828: Closing this issue.

In response to this:

I'll defer to @nojnhuh on this since most of these spec types were replaced with the ASO types. Closing it out for now, and if we need to reopen, we could probably just make a new issue specific to ASO.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Archived in project
Development

No branches or pull requests

8 participants