233 rework the provider configuration #254

d3dfx · 2025-05-21T03:57:23Z

Closes #233

Hello Maintainers, I'm new to contributing! Please, let me know what needs to change!

Signed-off-by: Evan Lankford <[email protected]>

maveonair

All tests are failing, please fix them before we dive into the review.

d3dfx · 2025-05-21T12:12:54Z

All tests are failing, please fix them before we dive into the review.

Yes Sir, this was dumb of me. I apologize, for wasting your time!

Signed-off-by: Evan Lankford <[email protected]>

maveonair

@d3dfx Please have a look at my comments.

Furthermore, please read our contribution guidelines and follow our commits structure: https://github.com/lxc/terraform-provider-incus/blob/main/CONTRIBUTING.md

maveonair · 2025-05-24T08:08:52Z

.github/workflows/test.yml

@@ -31,7 +31,8 @@ jobs:
      TF_ACC: "1"
      GO111MODULE: "on"
      INCUS_REMOTE: local
-      INCUS_ADDR: localhost
+      INCUS_ADDR: https://localhost:8443
+      INCUS_FQDN: localhost


Let's still use INCUS_ADDR, no need for INCUS_FQDN

maveonair · 2025-05-24T08:11:21Z

docs/index.md

@@ -74,29 +73,29 @@ The following arguments are supported:
  also be set with the `INCUS_ACCEPT_SERVER_CERTIFICATE` environment variable.
  Defaults to `false`

-The `remote` block supports:
+* `default_remote` - *Optional* - The `address` of the default remote to use.


I am not sure that this should be optional anymore but rather mandatory now.

Understood!

maveonair · 2025-05-24T08:12:05Z

internal/provider-config/config.go

@@ -376,22 +381,6 @@ func connectToIncusServer(instServer incus.InstanceServer) error {
 	return nil
 }

-// determineIncusDaemonAddr determines address of the Incusserver daemon.
-func determineIncusDaemonAddr(remote IncusProviderRemoteConfig) (string, error) {


Here I don't agree, it should be still possible to use the UNIX socket, not just the HTTPS endpoint.

I believe that it is still possible to use the unix socket by setting the unix scheme in the address like such --> address: unix:///my/unix/path

I will test using the unix:// address tonight to ensure i'm not full of it!

I can also make the unix scheme the default for addresses that dont have a scheme assigned. I think that would meet the intention of the function!

Okay, so it looks like using a unix path does work, I was able to create a test instance with the following config!

provider "incus" { generate_client_certificates = true accept_remote_certificate = true default_remote = "unix:///var/lib/incus/unix.socket" remote { name = "local" address = "unix:///var/lib/incus/unix.socket" token = "token" } }

Shouldn’t it be that when I set default_remote = "local", it selects one of the remote blocks defined below?

maveonair · 2025-05-24T08:12:47Z

internal/provider/provider.go

@@ -87,6 +89,10 @@ func (p *IncusProvider) Schema(_ context.Context, _ provider.SchemaRequest, resp
 				Optional:    true,
 				Description: "The project where project-scoped resources will be created. Can be overridden in individual resources. (default = default)",
 			},
+			"default_remote": schema.StringAttribute{
+				Optional:    true,
+				Description: "The `address` of the default remote to use.",


Suggested change

Description: "The `address` of the default remote to use.",

Description: "The default remote to use when no other remote is defined in a resource.",

maveonair · 2025-05-24T08:14:34Z

internal/provider/provider.go

 							Optional:    true,
-							Description: "Set this remote as default.",
+							Description: "Default project to configure. ( Only for the `incus` protocol )",


The default project is already used always by design, so this should not be needed. What was your intention here?

I added it as it was in the list of requested fields to add it in the ticket #233 .

I think the intention is to make it more similar to this command in the cli --> https://linuxcontainers.org/incus/docs/main/reference/manpages/incus/remote/add/#incus-remote-add-md

I can remove it, if you would like!

Okay, so this means we would make default_remote now required to be set?

maveonair · 2025-05-24T08:15:45Z

internal/provider/provider.go

 		}

-		scheme := remote.Scheme.ValueString()


As mentioned before, the UNIX socket connection should still be usable.

d3dfx · 2025-05-28T16:50:34Z

@d3dfx Please have a look at my comments.

Furthermore, please read our contribution guidelines and follow our commits structure: https://github.com/lxc/terraform-provider-incus/blob/main/CONTRIBUTING.md

I will re push my changes with only three commits! I apologize, I misunderstood it!

d3dfx and others added 9 commits May 20, 2025 23:58

Provider: update IncusProviderModel structs

8c68cfc

Signed-off-by: Evan Lankford <[email protected]>

Provider: update IncusProvider schemas

1f60157

Signed-off-by: Evan Lankford <[email protected]>

Provider: fix auth_type schema

e6d1187

Signed-off-by: Evan Lankford <[email protected]>

Provider: refactor initial remotes config

b9eee44

Signed-off-by: Evan Lankford <[email protected]>

Provider: add token back!

96d8276

Signed-off-by: Evan Lankford <[email protected]>

Provider: rm fn && add parse url to get addr schem

b389bb9

Signed-off-by: Evan Lankford <[email protected]>

Provider: mk remote name opt & use addr to assgn default_remote

b43118a

Signed-off-by: Evan Lankford <[email protected]>

Provider: update schema descriptions

1c7dbc4

Signed-off-by: Evan Lankford <[email protected]>

docs: update index to reflect provider changes

8ccc3fe

Signed-off-by: Evan Lankford <[email protected]>

d3dfx force-pushed the 233-rework-the-provider-configuration-2 branch from 16c24c0 to 8ccc3fe Compare May 21, 2025 04:01

maveonair self-requested a review May 21, 2025 04:55

maveonair requested changes May 21, 2025

View reviewed changes

maveonair requested a review from stgraber May 21, 2025 04:55

d3dfx force-pushed the 233-rework-the-provider-configuration-2 branch from 880830c to 3e7d24a Compare May 23, 2025 02:43

tests: Update Test pipeline with new ENV Vars

1db62ad

Signed-off-by: Evan Lankford <[email protected]>

d3dfx force-pushed the 233-rework-the-provider-configuration-2 branch from 3e7d24a to 1db62ad Compare May 23, 2025 23:39

maveonair requested changes May 24, 2025

View reviewed changes

	Description: "The `address` of the default remote to use.",
	Description: "The default remote to use when no other remote is defined in a resource.",

233 rework the provider configuration #254

Are you sure you want to change the base?

233 rework the provider configuration #254

Uh oh!

Conversation

d3dfx commented May 21, 2025

Uh oh!

maveonair left a comment

Choose a reason for hiding this comment

Uh oh!

d3dfx commented May 21, 2025

Uh oh!

maveonair left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maveonair Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3dfx commented May 28, 2025

Uh oh!

Uh oh!

maveonair Jun 9, 2025 •

edited

Loading