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

Feature: add ContainerConfig to Container #159

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

xortim
Copy link
Contributor

@xortim xortim commented Sep 5, 2024

Add ContainerConfig to Container similar to VIrtualMachineConfig

types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
nodes.go Show resolved Hide resolved
@luthermonson
Copy link
Owner

since your the first person in a while to touch the container code is there any chance I could convince you to get some data out of your pve environment and add some fixtures and build a test for the Container func?

@xortim
Copy link
Contributor Author

xortim commented Sep 9, 2024

@luthermonson - I'd be happy to add fixtures and tests for the Container functions. First I'll need to figure out how :)

@luthermonson
Copy link
Owner

https://github.com/luthermonson/go-proxmox/blob/main/tests/mocks/pve7x/nodes.go#L961

i think you need to fill in some data here and then add some more asserts into both this: https://github.com/luthermonson/go-proxmox/blob/main/containers_test.go#L74-L86

and probably make a TestContainer which asserts the config data is populated in the response

@xortim
Copy link
Contributor Author

xortim commented Sep 12, 2024

Thanks for the pointers. I'm using PVE 8, would it be appropriate to add this to https://github.com/luthermonson/go-proxmox/blob/main/tests/mocks/pve8x/nodes.go instead?

Since it it looks like not all fields are populated with the defaults, how complete do we want the fixtures to be?

@xortim xortim force-pushed the feature/container-config branch from f85ccbf to 567b234 Compare September 15, 2024 20:29
@luthermonson
Copy link
Owner

You know there should be some over haul now to have pve8 as the default... can you just add a fixture into pve8 so there is data and I can work on the overhaul? Thats good enough for now

@luthermonson
Copy link
Owner

luthermonson commented Sep 18, 2024

Wrote the comment before I read the code, I think what you have is sufficient. Let's move on and if you'd like to add the fixture from your pve8 env later we can work it into the process. My new task is to upgrade my env to pve8 and add all the fixtures we have in pve7 and then change the default to 8. I won't make you deal with that

@luthermonson luthermonson merged commit 003dbe8 into luthermonson:main Sep 18, 2024
1 check passed
@xortim
Copy link
Contributor Author

xortim commented Sep 19, 2024

@luthermonson, sorry for the confusion. I'll move the fixture out of pve7 and into pve8 in another PR

@xortim xortim deleted the feature/container-config branch September 19, 2024 23:46
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.

2 participants