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

[Issue 281] Fix pool config regression #282

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

mpywell
Copy link
Contributor

@mpywell mpywell commented Sep 28, 2024

A regression was introduced by the proxmox-api-go version used in v1.2.0, which was fixed in version v0.0.0-20240525163725-6676d8933df0. Some other API changes had occurred around Tags and QEMU Agent configuration which have been handled and tested as part of this PR.

Closes #281

@mpywell mpywell requested a review from a team as a code owner September 28, 2024 12:19
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing this @mpywell !

I left a small nit on the generateAgentConfig function, but outside of this, we're good to go.
I'll let you address the comment and I'll be back for a second pass after that.

Pre-approving to not delay merging later.

@@ -254,6 +250,30 @@ func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multist
return multistep.ActionContinue
}

func generateAgentConfig(agent config.Trilean) *proxmox.QemuGuestAgent {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to add more to this function? It seems to me that this could be simplified to &proxmox.QemuGuestAgent{Enable:agent.True()} otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did consider proxmox.QemuGuestAgent{Enable:agent.True()} at first but I had a look over the open issues and decided to set this up as a function so I could address #276 in another PR

@mpywell
Copy link
Contributor Author

mpywell commented Oct 3, 2024

Hey @lbajolet-hashicorp comment addressed, should be good to go.

@lbajolet-hashicorp
Copy link
Contributor

Sounds good, thanks for the update @mpywell!

Merging this one now, I'll release a new version of the plugin with that fix in since it's a regression introduced in 1.2.0, so 1.2.1 coming-up today hopefully!

@lbajolet-hashicorp lbajolet-hashicorp merged commit ac42888 into hashicorp:main Oct 3, 2024
12 checks passed
@mpywell mpywell deleted the fix/281 branch October 4, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Pool parameter is not respected in v1.2.0
2 participants