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

Fix module path from ddelnano to vatesfr #302

Closed
wants to merge 2 commits into from
Closed

Conversation

belfhi
Copy link
Contributor

@belfhi belfhi commented Feb 28, 2024

@ddelnano ddelnano mentioned this pull request Feb 28, 2024
@belfhi belfhi changed the title Changes go module path from ddelnano to vatesfr Fix module path from ddelnano to vatesfr and update go Feb 29, 2024
@belfhi belfhi marked this pull request as ready for review February 29, 2024 08:49
@belfhi
Copy link
Contributor Author

belfhi commented Feb 29, 2024

@ddelnano please run CI :)

go.mod Outdated
Comment on lines 3 to 5
go 1.20
go 1.22

toolchain go1.22.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

If upgrading the go version is required, it should be separated out into a different change. The github.com/hashicorp/terraform-plugin-sdk project is still on go 1.21, so I don't think we should upgrade to go 1.22 unless there is a strong need.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This also requires Jenkins side changes to upgrade the go version.

go-version: "1.20"
go-version: "1.22"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment here and for the other GitHub action jobs.

client/go.mod Outdated
Comment on lines 1 to 10
module github.com/ddelnano/terraform-provider-xenorchestra/client

go 1.20

require (
github.com/cenkalti/backoff/v3 v3.2.2
github.com/gorilla/websocket v1.4.2
github.com/mitchellh/mapstructure v1.5.0
github.com/sourcegraph/jsonrpc2 v0.0.0-20210201082850-366fbb520750
)
Copy link
Collaborator

@ddelnano ddelnano Feb 29, 2024

Choose a reason for hiding this comment

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

What's the rationale for removing this? This was done to support terraformer (#138) and there is also a strong likelihood that this code will be shared by the XO packer plugin.

The terraformer integration is not important, but the XO packer plugin will need this exact same code and the plan is to have this live outside of this repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well we had import errors when we built the pulumi provider that we could resolve by just having one go.md file.
I see the need for it now, but maybe moving it into another repository is the way to go then :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you share the steps for reproducing the pulumi build issue or share the output of building it against #309? Unless it it isn't possible to make the pulumi work coexist, this must stay for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@belfhi just checking in on my earlier message. Once that is answered, we can move quickly on merging the last PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @ddelnano so I can confirm that removing the client go.mod is not neccessary. I manages to build the pulumi provider https://github.com/belfhi/pulumi-xenorchestra using a rewrite for the revision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for confirming 👍

Comment on lines -210 to 211

# Updating the VM to use 3 CPUs would happen without stopping/starting the VM
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is cause the failing tfplugindocs run since it removed the same whitespace from the vm resource mardown file.

ddelnano@ddelnano-desktop:~/code/terraform-provider-xenorchestra (belfhi/master) $ ~/go/bin/tfplugindocs

ddelnano@ddelnano-desktop:~/code/terraform-provider-xenorchestra (belfhi/master) $ git diff docs/
diff --git a/docs/resources/vm.md b/docs/resources/vm.md
index e979d35..7972c31 100644
--- a/docs/resources/vm.md
+++ b/docs/resources/vm.md
@@ -129,7 +129,7 @@ $ xo-cli xo.getAllObjects filter='json:{"id": "cf7b5d7d-3cd5-6b7c-5025-5c935c8cd
   "max": 4,
   "number": 2
 }
-
+
 # Updating the VM to use 3 CPUs would happen without stopping/starting the VM
 # Updating the VM to use 5 CPUs would stop/start the VM

Please run go generate ./... from the root of the repo to make those changes locally.

@ddelnano ddelnano mentioned this pull request Mar 5, 2024
@ddelnano
Copy link
Collaborator

ddelnano commented Mar 5, 2024

@belfhi I've pulled out the go upgrade commit into #307 and will merge that once the jenkins build passes.

I did not remove the client/go.mod file because that needs to stay for now (will be used by another project and eventually split out to its own repo). Please rebase once #307 is merged and we can get the other changes in a second step.

@belfhi belfhi changed the title Fix module path from ddelnano to vatesfr and update go Fix module path from ddelnano to vatesfr Mar 6, 2024
@ddelnano
Copy link
Collaborator

This is complete with #307 and #309 merged. Thanks for the contribution @belfhi and looking forward to working more towards the pulumi integration!

@ddelnano ddelnano closed this Mar 13, 2024
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