search

metal-stack-cloud / api

MIT License
1 stars 0 forks source link

Machine work uint conversion #174

Open Honigeintopf opened 2 months ago

Honigeintopf commented 2 months ago

While fixxing the linter I asked myself why we convert so much from uint to int and other way arround.

Here for example, our api wants the workers to have minsize uint32(which makes more sense, why have negative value for that), but the gardener worker has int32.

https://github.com/metal-stack-cloud/api-server/blob/ddf42821ae81c8ae2157ab9ae2100e3542d6685c/pkg/gardener/shoot.go#L515-L523

https://github.com/gardener/gardener/blob/029dd818e2d2252238a6d8cf89193748a7c00a12/pkg/apis/core/v1beta1/types_shoot.go#L1469-L1495

Why don't we just use the gardener worker api values like int32 which helps us remove the cast. We can manually check if it's negative and just throw an error on that case?

Already asked in gardener slack, this is probably just some issue that could be ignored, but gotta make sure before putting a ignore to the linter.

Honigeintopf commented 2 months ago

What do you think? https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types @Gerrit91 @vknabel

vknabel commented 1 month ago

@Honigeintopf do you remember what we decided? If I remember correctly we decided to do nothing. In that case: can we close this issue?

Honigeintopf commented 1 month ago

I think we went through some options on how to fix it. If it's not possible that gardener gives us a negative value, because of int32 (maybe they do to signal sth being broken, not sure just theory), then we can put a linter ignore before it. Imo changing it would be the better approach, might be a breaking change tho?

vknabel commented 1 month ago

Yup, it would break almost all clients of the API (metal-cli, terraform, api-server). The web console isn't affected as there is only number in JS.