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 pulumi-xenorchestra community package #6763

Closed
wants to merge 1 commit into from

Conversation

gCyrille
Copy link
Contributor

@gCyrille gCyrille commented Feb 28, 2025

Description

Hello,

I work for vatesfr and here is the PR to add the pulumi-xenorchestra provider plugin to the community package list. We recently released the v1.4.0

I'm not sure if it's ready, but I tried to follow the "Authoring and publishing Pulumi package" guide.

Edit: I checked the list below and I am not sure about this:

  • Logo format -> It's an svg. Do I need to change it ?
  • The layout property for the docs/_index.md is "overview"

Help/Reviews are welcome to improve the package :)
Thanks

Adding a new package?

If this pull request adds a new package, please verify/document:

  • The primary maintainer contact for this package (please indicate if the provider is sponsored by a company or community maintained)
  • The package's schema URL in this PR is correct.
  • The package metadata file, if present, contains:
    • a supported category (one of Cloud, Infrastructure, Network, Database, Monitoring, or Utility).
    • a valid plugin download URL. See Publish Your Package.
    • a description that explains what the package does.
    • a valid logo URL that points to a PNG whose dimensions conform to the others in this repo (e.g., 100x100).
  • The package repo contains an Overview doc (/docs/_index.md) that includes:
    • a brief explanation of what the package is and what it does.
    • at least one representative example in all supported languages (i.e. TypeScript, Python, Go and C#).
    • a front-matter property for the layout set to package.
  • The package repo contains an Installation and Configuration doc (/docs/installation-configuration.md) that includes:
    • links to SDKs in all supported languages (i.e. should link to published SDKs in NPM, PyPi and NuGet).
    • a copyable command for installing the resource plugin if necessary.
    • an example of configuring the provider with pulumi config set.
    • an example of configuring the provider with environment variables.
  • The repository has:
    • a version tag prefixed with v that corresponds with a valid GitHub release and published package SDKs (in NPM, PyPi, and NuGet)
  • A CODEOWNER has reviewed the PR.
  • A member of the @pulumi/docs team has reviewed all documentation.

@gCyrille gCyrille requested review from sean1588 and a team as code owners February 28, 2025 15:43
@flostadler
Copy link
Contributor

flostadler commented Feb 28, 2025

Hey @gCyrille, thanks a lot for adding this new provider! 🚀
I'll verify the provider and configuration before adding the provider to the registry. Will you be the contact person for this provider?

I’ll work through this checklist to confirm it’s ready to go in.

Checklist

  • Pulumi has appropriate contact information from the provider maintainer

  • The package will generate accurate documentation:

    1. Check out the PR under review and run:
    $ make bin/resourcedocsgen
    $ ./bin/resourcedocsgen metadata from-github \
            --repo-slug '<repoSlug>' \
            --schemaFile '<schemaFile>' \
            --version '<version>'

    Here <repoSlug> and <schemaFile> should match exactly the values added to /community-packages/package-list.json.
    This will generate metadata for the provider locally.

    1. Push the metadata files into a PR (either back to the PR under review or a new PR).
    • Confirm that that CI passes for the PR with the metadata files.
    • Click through the site preview and confirm that the docs (for the new provider) render as expected.
    • The registry renders a valid logo for the new provider.
  • Hand-written docs are complete and accurate:

    • Validate that you can install the new provider with the instructions found in /docs/installation-configuration.md.
      Maintainers should run the pulumi plugin install resource <name> <version> --server <pluginDownloadURL> command specified in the /docs/installation-configuration.md and see a provider be downloaded.
    • /docs/installation-configuration.md contains a link to the published SDK in each language (i.e. TypeScript, Python, Go and C#).
    • /docs/_index.md contains a minimal example in every supported language.
    • /docs/_index.md contains a brief explanation of what the package does.
  • There is a published version:

    • The repository has a version tag prefixed with v that corresponds with a valid GitHub release
    • Each published SDK has a matching release
  • A CODEOWNER has approved the PR.

@mjeffryes
Copy link
Member

mjeffryes commented Feb 28, 2025

Logo format -> It's an svg. Do I need to change it ?

I believe yes, you'll need it to be a png or it will get rejected by NuGet (the dotnet package manager)

@gCyrille
Copy link
Contributor Author

gCyrille commented Mar 3, 2025

Hi @flostadler, do you need a contact email address? If so, I can give you the email of our devops team who will provide support. Otherwise it can be me.

@flostadler
Copy link
Contributor

@gCyrille the email of the devops team sounds good, thanks!

Here's a preview of the rendered registry page1 for the xenorchestra provider.

The SVG icon renders correctly in the registry, but as @mjeffryes pointed out, I'd recommend using a png instead because NuGet needs one anyways.

In general the docs are looking good and the provider can be installed by following the instructions. I see you left a TODO for adding the dotnet SDK, installation instructions and example. Once that's added we should be good to add the provider to the registry.
Additionally I'd recommend you to unify the examples for the different languages1. They mix different styles of configuring the provider (TS has no indications about provider configuration, Python uses an explicit provider and Go calls out the config options users need to set for the default provider).

Footnotes

  1. This link might go away, since its to a temporary copy of the registry spun up for Test vatesfr/pulumi-xenorchestra #6772. 2

@gCyrille
Copy link
Contributor Author

gCyrille commented Mar 3, 2025

I made a new release (v1.4.1) that include a valid logo (png 100x100)

@gCyrille
Copy link
Contributor Author

gCyrille commented Mar 3, 2025

Regarding the dotnet SDK, there was a bug with wrong type conversion in previous releases (a int field in the Golang-terraform provider was converted to int in dotnet with only 32bits, which doesn't work for vdi sizes in bytes...).

I don't know if we can fix this in the near future. Is it possible to publish the provider without dotnet ?

@flostadler
Copy link
Contributor

flostadler commented Mar 3, 2025

@gCyrille the issue with 64bit integers requires pulumi/pulumi#14242 to be resolved. But this requires some deeper changes to the state and wire protocols. I don't think a near future fix for this is realistic.

One possible workaround for provider authors is to change the offending properties to be string encoded numbers instead. (Note, javascript also does not support the full 64bit width. There the maximum safe integer is 2^53 - 1, see).

We do have a policy that new providers should have SDKs published in all Pulumi languages and that the examples on the docs pages support all Pulumi languages as well. Would you be willing to publish a dotnet SDK or is the 32bit limitation severe/destructive enough to omit the whole SDK?

@gCyrille
Copy link
Contributor Author

gCyrille commented Mar 3, 2025

@flostadler The 32bit limitation is severe because it limits the VM disk sizes to 2GB. We would like to push the provider without dotnet support, it seems better than a buggy dotnet sdk.

@flostadler
Copy link
Contributor

@gCyrille I've brought this up internally, but we've decided to stick to the rule of requiring dotnet SDKs. I'm sorry!

What do you think about overriding the RAM properties in pulumi and changing them from ints to strings? The added benefit here would be that you can also accept units to allow things like specifying "8GB".

@gCyrille
Copy link
Contributor Author

gCyrille commented Mar 4, 2025

If it's possible to override the property in Pulumi to allow format like "8GB" that would be great! How can I do that? Can you share some documentation?

@flostadler
Copy link
Contributor

We don't have docs for that afaik (passing that feedback along internally to the right team).

You'd need to overwrite the type here: https://github.com/vatesfr/pulumi-xenorchestra/blob/f74f675c6d154e834a54213c372e28fb540371f3/provider/resources.go#L127

e.g. this would turn into something like this:

"xenorchestra_vm": {
    Tok: tfbridge.MakeResource(mainPkg, mainMod, "Vm"),
    Fields: map[string]*tfbridge.SchemaInfo{
	  "memory_max": {
		  Type:      "string",
		  Transform: func(pv resource.PropertyValue) (resource.PropertyValue, error) { /** Convert the string representation to an int **/},
	  },
    },
},

Alternatively, you could just change the type to "number". This would use a float64 under the hood. That should at least have 2^52 bits of precision, which should work for memory as well. E.g.

"xenorchestra_vm": {
    Tok: tfbridge.MakeResource(mainPkg, mainMod, "Vm"),
    Fields: map[string]*tfbridge.SchemaInfo{
	  "memory_max": {
		  Type:      "number",
	  },
    },
},

Happy to help (feel free to ping me on the community slack) if you need some help here

@gCyrille
Copy link
Contributor Author

gCyrille commented Mar 7, 2025

@flostadler Thanks for the help.
We released version 1.5.0 with dotnet support and unified examples :)

@flostadler
Copy link
Contributor

Awesome @gCyrille, taking a look now!

@flostadler
Copy link
Contributor

Here's the new preview: http://registry--origin-pr-6852-10fdf90e.s3-website.us-west-2.amazonaws.com/registry/packages/xenorchestra/

Working through the checklist now

@gCyrille
Copy link
Contributor Author

@flostadler the examples for the Dotnet are missing from the preview. Do I need to fix something?

@flostadler
Copy link
Contributor

@gCyrille You're fast! Was just about to bring that up. The language chooser for dotnet needs to be called csharp, we'll be adding more docs for how to author those examples (#6775).

Here's an example from another provider: https://github.com/pulumi/registry/blob/master/themes/default/content/registry/packages/talos/_index.md?plain=1

@flostadler
Copy link
Contributor

@gCyrille I just noticed that xen-orchestra is AGPL licensed. The provider itself is Apache licensed from what I can see, is that right?
Just wanted to confirm that with you and check that there's no AGPL licensed code that found its way into the provider

@flostadler
Copy link
Contributor

@gCyrille
Copy link
Contributor Author

It should be good in v1.5.2

I confirm for the licenses, that no code of Xen Orchestra has found its way into the provider :)

@flostadler
Copy link
Contributor

Thanks @gCyrille, I'm currently confirming the addition of the xenorchestra package internally and will let you know ASAP what the next steps are

@flostadler flostadler mentioned this pull request Mar 20, 2025
14 tasks
@flostadler
Copy link
Contributor

@gCyrille I added the xenorchestra provider in #6852. Going to close this PR now

@flostadler flostadler closed this Mar 20, 2025
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