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

✨ Ability to use VCPUs #136

Merged
merged 3 commits into from
May 31, 2024
Merged

✨ Ability to use VCPUs #136

merged 3 commits into from
May 31, 2024

Conversation

Mattes83
Copy link
Contributor

What is the purpose of this pull request/Why do we need it?
This allows to use server type VCPU instead of ENTERPRISE.

Issue #, if available:
#129

Description of changes:
Added new type field "Type" (default ENTERPRISE) to IonosCloudMachineSpec and use it in buildServerProperties.

@lubedacht
Copy link
Contributor

lubedacht commented May 27, 2024

For the next time, please stick to the style guide when adding additional commits to a PR

- Don’t force-push your git branch after someone started reviewing it. Changing the history makes it harder for a reviewer to find the exact diff, e.g. since the reviewer had a look the last time.

@Mattes83
Copy link
Contributor Author

Ah sorry for that. I see I should not have resolved your comments either.

@lubedacht
Copy link
Contributor

I just noticed an issue with the tests. I was confused that there are no updated tests in server_test.go. The mockCreateServer call was done with mock.Anything and this is actually not good. We should have an expectation and we should have tests for VCPU and Enterprise.

Would you be so kind to update the tests so we can ensure that the correct values are passed to the CreateServer call?
I know this will require to update multiple tests but will definitely improve our test quality.

@Mattes83
Copy link
Contributor Author

I am now testing for VCPU or ENTERPRISE

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Mattes83
Copy link
Contributor Author

I guess I handled all remarks, please let me know if there is more I should change

@Mattes83
Copy link
Contributor Author

@lubedacht I guess I can merge the PR now?

@lubedacht lubedacht merged commit 98b9e63 into main May 31, 2024
9 checks passed
@lubedacht lubedacht deleted the feature_vcpus branch May 31, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants