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

🌱 Add new CX server types #1330

Merged
merged 1 commit into from
Jun 11, 2024
Merged

🌱 Add new CX server types #1330

merged 1 commit into from
Jun 11, 2024

Conversation

apricote
Copy link
Contributor

@apricote apricote commented Jun 6, 2024

What this PR does / why we need it:

Adds the new server types cx22, cx32, cx42 and cx52 to the CRD validation. (There is no cx12). These were just launched and you can find out more at https://www.hetzner.com/news/new-cx-plans/

The previous generation (cx11 - cx51) was deprecated and will be removed on 06 September 2024. I saw that you reference them in some templates and you might want to replace them until then.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squash commits
  • include documentation
  • add unit tests

@syself-bot syself-bot bot added size/XS Denotes a PR that changes 0-20 lines, ignoring generated files. area/api Changes made in the api directory labels Jun 6, 2024
Copy link
Contributor

@janiskemper janiskemper left a comment

Choose a reason for hiding this comment

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

thanks! @kranurag7 can you have a look and update the templates?

@janiskemper
Copy link
Contributor

@apricote general question: What about the fact that certain server types are only available in some locations? Do you think it is feasible that we make an actual validation so that people can be 100% sure that the type exists? For example with the US regions?

Copy link
Contributor

@kranurag7 kranurag7 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you Julian for the patch.

@kranurag7 kranurag7 merged commit 1d2d9c8 into syself:main Jun 11, 2024
9 checks passed
@apricote
Copy link
Contributor Author

What about the fact that certain server types are only available in some locations? Do you think it is feasible that we make an actual validation so that people can be 100% sure that the type exists? For example with the US regions?

Thats certainly a possibility to do in the validating webhook. This makes it dependent on the Cloud API, not sure if you want that, though a 1 hour cache or similar would be good enough.

You would also need some kind of API Token to get the list of server types. As far as I can see, there is no direct link between the HcloudMachineTemplate & the tokens. There is also no guarantee, that every user/project has the same server types available.

@janiskemper
Copy link
Contributor

I actually thought about hardcoding it in CAPH 😅
So kinda having what we already have in little more complex with taking into account the different regions.

What do you mean by this?

There is also no guarantee, that every user/project has the same server types

I'm just looking for a solution that makes it the most reliable that users don't specify server types that are not available

@apricote
Copy link
Contributor Author

There is also no guarantee, that every user/project has the same server types

There might be users with pre-view access to new server types, or custom server types for their usecases. I think this is mostly used internally, for example before the CAX servers launched the employees already had access to develop & test for them.

This is already an issue with the current hardcoded enum in the CRD, so whatever you are going to change its not going to get worse :D

Hardcoding it somewhere is probably the easiest. If its in the CRD, editors/IDEs can get the json schema and provide the user with autocomplete, not sure if any editor works well with this.

@apricote apricote deleted the new-cx-types branch June 14, 2024 13:29
@janiskemper
Copy link
Contributor

So what would be your ideal solution? Not validating? If validating then only with an API call to fetch the "real" and up-to-date list? Having a more complex logic per location? Not validation but some good documentation so that users don't do mistakes?
Currently, I'm not even sure if it is visible in a good way if a type that is not available is specified and the server cannot create it. Need to check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Changes made in the api directory size/XS Denotes a PR that changes 0-20 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants