Add source_file to incus_image #169

breml · 2024-11-20T10:13:07Z

Resolves: #125

internal/image/resource_image.go

breml · 2024-11-20T10:16:15Z

internal/image/resource_image.go

+	// NOTE: this does not seem to have an effect
+	// imageAliases := make([]api.ImageAlias, 0, len(aliases))
+	// for _, alias := range aliases {
+	// 	// Ensure image alias does not already exist.
+	// 	aliasTarget, _, _ := server.GetImageAlias(alias)
+	// 	if aliasTarget != nil {
+	// 		resp.Diagnostics.AddError(fmt.Sprintf("Image alias %q already exists", alias), "")
+	// 		return
+	// 	}
+
+	// 	ia := api.ImageAlias{
+	// 		Name: alias,
+	// 	}
+
+	// 	imageAliases = append(imageAliases, ia)
+	// }
+	// image.Aliases = imageAliases


I expected incus to respect the Aliases provided in the api.ImagesPost request, but this is currently not the case. Therefore I added the aliases manually, see https://github.com/lxc/terraform-provider-incus/pull/169/files#diff-dedceb2079bdc5668f6c75a9b22a13d869a760a187b2804ac22f83eeac6d50e7R611

Not sure, about the correct thing to do here.

Pretty sure the server does, but the client code wants the extra Aliases passed through args.Aliases ImageCopyArgs

lxc/incus#1409 switches our CLI client to actually using this feature and it seems to be working just fine

What do you mean with it does not respect the aliases provided? because when I set the aliases in a incus_image then I see those created:

resource "incus_image" "alpine" { project = "default" aliases = ["alpine", "alpine/edge"] source_image = { remote = "images" name = "alpine/edge" type = "container" architecture = "aarch64" } }

$ incus image list +-----------------+--------------+--------+------------------------------------+--------------+-----------+---------+----------------------+ | ALIAS | FINGERPRINT | PUBLIC | DESCRIPTION | ARCHITECTURE | TYPE | SIZE | UPLOAD DATE | +-----------------+--------------+--------+------------------------------------+--------------+-----------+---------+----------------------+ | alpine (1 more) | 57c20fddd750 | no | Alpine edge arm64 (20241119_13:00) | aarch64 | CONTAINER | 3.31MiB | 2024/11/22 22:50 CET | +-----------------+--------------+--------+------------------------------------+--------------+-----------+---------+----------------------+ $ incus image list -f json | jq .[].aliases [ { "name": "alpine", "description": "" }, { "name": "alpine/edge", "description": "" } ]

I tested the import from source_file with inucs from mainbranch (including lxc/incus#1409) and the acceptance test is not successful, if I use the commented out section from above without the manual adding of the aliases.

The reason in my opinion is, that the import from_source file does use client.CreateImage directly and this function does still not respect the Aliases. The fix from lxc/incus#1409 does change it for incus image copy, but not for incus image import. There it is still done as part of the code of the cmd in the incus cli and not in the client.

Ah right, that's because incus image import doesn't get to send an API struct to the server as we instead send raw image data and rely on headers for the rest.

So having the logic applying the aliases after import makes sense.

We should be able to extend the API to allow for aliases to be passed as HTTP headers too but that still won't be as flexible if we ever want to support alias re-use like we do in the CLI now.

lxc/incus#1434

@stgraber thanks for the addition in lxc/incus#1434. This will become available with incus 6.8, which is not yet released. But for now, this is not available and I assume, that the incus terraform provider should also be backwards compatible and work with older versions of incus. Therefore I assume, that we stick to the current implementation. Do you agree?
CC: @maveonair

Normally I'd agree but given that this is a new feature which won't be in a stable version of the provider for another few weeks, combined with Incus 6.8 being released next week, I think we can just rely on it.

On the Incus side, only the latest release is supported so 6.7 will no longer be officially supported once 6.8 comes out. The exception there is the LTS release, but 6.0.3 will come out a week after 6.8 and will include this API improvement, so we'll have it everywhere we care about.

OK, I updated the code to use aliases directly for the new feature and I successfully tested the change locally with incus from main and a go.work setup, such that the terraform provider also uses incus from main.

The tests in CI now fail, since the terraform provider is still using the old version of incus: https://github.com/lxc/terraform-provider-incus/actions/runs/12134741456/job/33832497665?pr=169#step:8:157

@stgraber there are some more locations, where the aliases are set explicitly instead of using the aliases from api.ImagesPost.

internal/image/resource_image_test.go

internal/image/resource_image.go

breml · 2024-11-22T09:33:32Z

@maveonair I know, this PR is still in draft, but it works on my machine as well as in CI and therefore I would appreciate some feedback, especially if I have missed something.

internal/image/resource_image.go

maveonair

@breml Very well done!

stgraber · 2024-12-04T06:03:17Z

=== RUN   TestAccImage_sourceFileSplitImage
    resource_image_test.go:336: Step 3/3 error: Error running apply: exit status 1
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to incus_image.from_file, provider
        "provider[\"registry.terraform.io/hashicorp/incus\"]" produced an unexpected
        new value: .aliases: was cty.SetVal([]cty.Value{cty.StringVal("able-snail"),
        cty.StringVal("teaching-spider")}), but now null.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
--- FAIL: TestAccImage_sourceFileSplitImage (7.01s)
=== RUN   TestAccImage_sourceFileUnifiedImage
    resource_image_test.go:385: Step 2/2 error: Error running apply: exit status 1
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to incus_image.from_file, provider
        "provider[\"registry.terraform.io/hashicorp/incus\"]" produced an unexpected
        new value: .aliases: was
        cty.SetVal([]cty.Value{cty.StringVal("destined-skylark"),
        cty.StringVal("sharing-terrier")}), but now null.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
--- FAIL: TestAccImage_sourceFileUnifiedImage (6.88s)

breml · 2024-12-04T06:14:27Z

=== RUN   TestAccImage_sourceFileSplitImage
    resource_image_test.go:336: Step 3/3 error: Error running apply: exit status 1
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to incus_image.from_file, provider
        "provider[\"registry.terraform.io/hashicorp/incus\"]" produced an unexpected
        new value: .aliases: was cty.SetVal([]cty.Value{cty.StringVal("able-snail"),
        cty.StringVal("teaching-spider")}), but now null.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
--- FAIL: TestAccImage_sourceFileSplitImage (7.01s)
=== RUN   TestAccImage_sourceFileUnifiedImage
    resource_image_test.go:385: Step 2/2 error: Error running apply: exit status 1
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to incus_image.from_file, provider
        "provider[\"registry.terraform.io/hashicorp/incus\"]" produced an unexpected
        new value: .aliases: was
        cty.SetVal([]cty.Value{cty.StringVal("destined-skylark"),
        cty.StringVal("sharing-terrier")}), but now null.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
--- FAIL: TestAccImage_sourceFileUnifiedImage (6.88s)

@stgraber Yes, I know. I commented about this in #169 (comment)

stgraber · 2024-12-04T06:39:26Z

Ah, right, that makes sense, though the Terraform error here is absolutely horrible ;)

stgraber · 2024-12-04T06:40:05Z

PR blocked until Incus 6.8 is out.

breml · 2024-12-16T05:53:44Z

Incus 6.8 is release: https://github.com/lxc/incus/releases/tag/v6.8.0

breml · 2024-12-16T06:26:56Z

CI pipeline is currently failing, because virtualization tests can not run on GH actions, see also: https://github.com/lxc/terraform-provider-incus/pull/186/files#r1886242549

maveonair · 2024-12-16T17:42:14Z

@breml Please merge origin main or rebase from origin main to get the latest fixes required for CI/CD to work.

Signed-off-by: Lucas Bremgartner <lucas.bremgartner@futurfusion.io>

breml · 2024-12-17T05:44:48Z

@maveonair I rebased on origin main.

breml commented Nov 20, 2024

View reviewed changes

internal/image/resource_image.go Outdated Show resolved Hide resolved

breml commented Nov 20, 2024

View reviewed changes

breml force-pushed the issue-125 branch from 24f7206 to d432b88 Compare November 20, 2024 10:18

breml commented Nov 20, 2024

View reviewed changes

internal/image/resource_image_test.go Outdated Show resolved Hide resolved

breml force-pushed the issue-125 branch 2 times, most recently from 532ca52 to cc4f4fc Compare November 20, 2024 11:01

breml commented Nov 21, 2024

View reviewed changes

internal/image/resource_image.go Outdated Show resolved Hide resolved

breml force-pushed the issue-125 branch 2 times, most recently from ee40de2 to 80b786c Compare November 21, 2024 08:15

breml marked this pull request as ready for review November 26, 2024 06:37

breml force-pushed the issue-125 branch from 80b786c to 5bc3162 Compare November 26, 2024 06:40

breml requested review from stgraber and maveonair November 26, 2024 06:41

breml force-pushed the issue-125 branch from 5bc3162 to fa85fe7 Compare December 2, 2024 07:02

stgraber reviewed Dec 2, 2024

View reviewed changes

internal/image/resource_image.go Outdated Show resolved Hide resolved

breml force-pushed the issue-125 branch from fa85fe7 to 780562d Compare December 3, 2024 07:16

maveonair approved these changes Dec 3, 2024

View reviewed changes

breml requested a review from stgraber December 3, 2024 07:29

stgraber approved these changes Dec 4, 2024

View reviewed changes

stgraber enabled auto-merge December 4, 2024 02:32

stgraber added the Blocked Waiting on an external task label Dec 4, 2024

auto-merge was automatically disabled December 16, 2024 05:55
Head branch was pushed to by a user without write access

breml force-pushed the issue-125 branch from 780562d to d4a2b37 Compare December 16, 2024 05:55

breml mentioned this pull request Dec 16, 2024

gomod: Update dependencies #186

Merged

breml removed the Blocked Waiting on an external task label Dec 16, 2024

breml added 3 commits December 17, 2024 06:43

docs: Add source_file to incus_image

04715fd

Signed-off-by: Lucas Bremgartner <lucas.bremgartner@futurfusion.io>

config: Add source_file to incus_image

d351651

Signed-off-by: Lucas Bremgartner <lucas.bremgartner@futurfusion.io>

tests: Add source_file to incus_image

1b233c7

Signed-off-by: Lucas Bremgartner <lucas.bremgartner@futurfusion.io>

breml force-pushed the issue-125 branch from d4a2b37 to 1b233c7 Compare December 17, 2024 05:44

stgraber merged commit 28f59a5 into lxc:main Dec 17, 2024
10 checks passed

breml deleted the issue-125 branch December 17, 2024 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source_file to incus_image #169

Add source_file to incus_image #169

breml commented Nov 20, 2024 •

edited

Loading

breml Nov 20, 2024

stgraber Nov 22, 2024

stgraber Nov 22, 2024

maveonair Nov 22, 2024 •

edited

Loading

breml Nov 25, 2024

stgraber Nov 28, 2024

stgraber Nov 28, 2024

breml Dec 2, 2024

stgraber Dec 2, 2024

breml Dec 3, 2024

breml commented Nov 22, 2024

maveonair left a comment

stgraber commented Dec 4, 2024

breml commented Dec 4, 2024

stgraber commented Dec 4, 2024

stgraber commented Dec 4, 2024

breml commented Dec 16, 2024

breml commented Dec 16, 2024

maveonair commented Dec 16, 2024

breml commented Dec 17, 2024

Add source_file to incus_image #169

Add source_file to incus_image #169

Conversation

breml commented Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maveonair Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breml commented Nov 22, 2024

maveonair left a comment

Choose a reason for hiding this comment

stgraber commented Dec 4, 2024

breml commented Dec 4, 2024

stgraber commented Dec 4, 2024

stgraber commented Dec 4, 2024

breml commented Dec 16, 2024

breml commented Dec 16, 2024

maveonair commented Dec 16, 2024

breml commented Dec 17, 2024

breml commented Nov 20, 2024 •

edited

Loading

maveonair Nov 22, 2024 •

edited

Loading