Merge pull request #414 from hmlanigan/fix-upgrade-revision

Fix upgrade charm revision for application resources

internal/juju/applications.go

@@ -1075,13 +1075,24 @@ func (c applicationsClient) computeSetCharmConfig(
 		return nil, err
 	}
 
+	// You can only refresh on the revision OR the channel at once.
 	newURL := oldURL
+	newOrigin := oldOrigin
 	if input.Revision != nil {
 		newURL = oldURL.WithRevision(*input.Revision)
-	}
-
-	newOrigin := oldOrigin
-	if input.Channel != "" {
+		newOrigin.Revision = input.Revision
+		// If the charm has an ID and Hash, it's been deployed before.
+		// Remove to trick juju into finding the new revision the user
+		// has requested. If they exist, the charm will be resolved with
+		// the channel potentially causing the wrong charm revision to
+		// be installed.
+		//
+		// There is a risk if the charm has been renamed in charmhub that
+		// the resolve charm will fail as we're using the name instead of
+		// the ID. This needs to be fixed in Juju.
+		newOrigin.ID = ""
+		newOrigin.Hash = ""
+	} else if input.Channel != "" {
 		parsedChannel, err := charm.ParseChannel(input.Channel)
 		if err != nil {
 			return nil, err
@@ -1095,18 +1106,40 @@ func (c applicationsClient) computeSetCharmConfig(
 		}
 	}
 
-	resolvedURL, resolvedOrigin, _, err := resolveCharm(charmsAPIClient, newURL, newOrigin)
+	resolvedURL, resolvedOrigin, supportedBases, err := resolveCharm(charmsAPIClient, newURL, newOrigin)
 	if err != nil {
 		return nil, err
 	}
 
-	resultOrigin, err := charmsAPIClient.AddCharm(resolvedURL, resolvedOrigin, false)
+	// Ensure that the new charm supports the architecture and
+	// operating system currently used by the deployed application.
+	if oldOrigin.Architecture != resolvedOrigin.Architecture {
+		msg := fmt.Sprintf("the new charm does not support the current architecture %q", oldOrigin.Architecture)
+		return nil, errors.New(msg)
+	}
+	if !basesContain(oldOrigin.Base, supportedBases) {
+		msg := fmt.Sprintf("the new charm does not support the current operating system %q", oldOrigin.Base.String())
+		return nil, errors.New(msg)
+	}
+
+	// Ensure the new revision or channel is contained
+	// in the origin to be saved by juju when AddCharm
+	// is called.
+	if input.Revision != nil {
+		oldOrigin.Revision = input.Revision
+	} else if input.Channel != "" {
+		oldOrigin.Track = newOrigin.Track
+		oldOrigin.Risk = newOrigin.Risk
+		oldOrigin.Branch = newOrigin.Branch
+	}
+
+	resultOrigin, err := charmsAPIClient.AddCharm(resolvedURL, oldOrigin, false)
 	if err != nil {
 		return nil, err
 	}
 
 	apiCharmID := apiapplication.CharmID{
-		URL:    newURL,
+		URL:    resolvedURL,
 		Origin: resultOrigin,
 	}
 

internal/provider/resource_application.go

@@ -629,9 +629,14 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq
 		}
 		planCharm := planCharms[0]
 		stateCharm := stateCharms[0]
-		if !planCharm.Channel.Equal(stateCharm.Channel) {
+		if !planCharm.Channel.Equal(stateCharm.Channel) && !planCharm.Revision.Equal(stateCharm.Revision) {
+			resp.Diagnostics.AddWarning("Not Supported", "Changing an application's revision and channel at the same time.")
+		} else if !planCharm.Channel.Equal(stateCharm.Channel) {
 			updateApplicationInput.Channel = planCharm.Channel.ValueString()
+		} else if !planCharm.Revision.Equal(stateCharm.Revision) {
+			updateApplicationInput.Revision = intPtr(planCharm.Revision)
 		}
+
 		if !planCharm.Series.Equal(stateCharm.Series) || !planCharm.Base.Equal(stateCharm.Base) {
 			// This violates terraform's declarative model. We could implement
 			// `juju set-application-base`, usually used after `upgrade-machine`,
@@ -642,9 +647,6 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq
 			// change to series, revision and channel at the same time.
 			resp.Diagnostics.AddWarning("Not Supported", "Changing an application's operating system after deploy.")
 		}
-		if !planCharm.Revision.Equal(stateCharm.Revision) {
-			updateApplicationInput.Revision = intPtr(planCharm.Revision)
-		}
 	}
 
 	if !plan.Expose.Equal(state.Expose) {

internal/provider/resource_application_test.go

@@ -9,6 +9,8 @@ import (
 
 	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
+
+	internaltesting "github.com/juju/terraform-provider-juju/internal/testing"
 )
 
 func TestAcc_ResourceApplication(t *testing.T) {
@@ -137,6 +139,37 @@ func TestAcc_ResourceApplication_Updates(t *testing.T) {
 	})
 }
 
+// TestAcc_ResourceApplication_UpdatesRevisionConfig will test the revision update that have new config parameters on
+// the charm. The test will check that the config is updated and the revision is updated as well.
+func TestAcc_ResourceApplication_UpdatesRevisionConfig(t *testing.T) {
+	if testingCloud != LXDCloudTesting {
+		t.Skip(t.Name() + " only runs with LXD")
+	}
+	modelName := acctest.RandomWithPrefix("tf-test-application")
+	appName := "github-runner"
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 func() { testAccPreCheck(t) },
+		ProtoV6ProviderFactories: frameworkProviderFactories,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccResourceApplicationWithRevisionAndConfig(modelName, appName, 88, ""),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("juju_application."+appName, "model", modelName),
+					resource.TestCheckResourceAttr("juju_application."+appName, "charm.#", "1"),
+					resource.TestCheckResourceAttr("juju_application."+appName, "charm.0.name", appName),
+					resource.TestCheckResourceAttr("juju_application."+appName, "charm.0.revision", "88"),
+				),
+			},
+			{
+				Config: testAccResourceApplicationWithRevisionAndConfig(modelName, appName, 95, ""),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("juju_application."+appName, "charm.0.revision", "95"),
+				),
+			},
+		},
+	})
+}
+
 func TestAcc_CharmUpdates(t *testing.T) {
 	modelName := acctest.RandomWithPrefix("tf-test-charmupdates")
 
@@ -290,6 +323,40 @@ func testAccResourceApplicationBasic(modelName, appName string) string {
 	}
 }
 
+func testAccResourceApplicationWithRevisionAndConfig(modelName, appName string, revision int, configParamName string) string {
+	return internaltesting.GetStringFromTemplateWithData(
+		"testAccResourceApplicationWithRevisionAndConfig",
+		`
+resource "juju_model" "{{.ModelName}}" {
+  name = "{{.ModelName}}"
+}
+
+resource "juju_application" "{{.AppName}}" {
+  name  = "{{.AppName}}"
+  model = juju_model.{{.ModelName}}.name
+
+  charm {
+    name     = "{{.AppName}}"
+    revision = {{.Revision}}
+    channel  = "latest/edge"
+  }
+
+  {{ if ne .ConfigParamName "" }}
+  config = {
+    {{.ConfigParamName}} = "{{.ConfigParamName}}-value"
+  }
+  {{ end }}
+
+  units = 1
+}
+`, internaltesting.TemplateData{
+			"ModelName":       modelName,
+			"AppName":         appName,
+			"Revision":        fmt.Sprintf("%d", revision),
+			"ConfigParamName": configParamName,
+		})
+}
+
 func testAccResourceApplicationUpdates(modelName string, units int, expose bool, hostname string) string {
 	exposeStr := "expose{}"
 	if !expose {

internal/testing/plangenerator.go

@@ -0,0 +1,84 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the Apache License, Version 2.0, see LICENCE file for details.
+
+package testsing
+
+import (
+	"bytes"
+	"text/template"
+)
+
+type TemplateData map[string]string
+
+// GetStringFromTemplateWithData returns a string from a template with data.
+//
+// Here is the example usage:
+// templateStr := GetStringFromTemplateWithData(
+//
+//	"testAccResourceApplicationWithRevisionAndConfig",
+//	`
+//
+//	resource "juju_model" "this" {
+//	 name = "{{.ModelName}}"
+//	}
+//
+//	resource "juju_application" "{{.AppName}}" {
+//	 name  = "{{.AppName}}"
+//	 model = juju_model.this.name
+//
+//	 charm {
+//	   name     = "{{.AppName}}"
+//	   revision = {{.Revision}}
+//	   channel  = "latest/edge"
+//	 }
+//
+//	 {{ if ne .ConfigParamName "" }}
+//	 config = {
+//	   {{.ConfigParamName}} = "{{.ConfigParamName}}-value"
+//	 }
+//	 {{ end }}
+//
+//	 units = 1
+//	}
+//
+//	`, utils.TemplateData{
+//					"ModelName":       "test-model",
+//					"AppName":         "test-app"
+//					"Revision":        fmt.Sprintf("%d", 7),
+//					"ConfigParamName": "test-config-param-name",
+//				})
+//
+// The templateStr will be:
+//
+//	resource "juju_model" "this" {
+//	  name = "test-model"
+//	}
+//
+//	resource "juju_application" "test-app" {
+//	  name  = "test-app"
+//	  model = juju_model.this.name
+//
+//	  charm {
+//	    name     = "test-app"
+//	    revision = 7
+//	    channel  = "latest/edge"
+//	  }
+//
+//	  config = {
+//	    test-config-param-name = "test-config-param-name-value"
+//	  }
+//
+//	  units = 1
+//	}
+func GetStringFromTemplateWithData(templateName string, templateStr string, templateData TemplateData) string {
+	tmpl, err := template.New(templateName).Parse(templateStr)
+	if err != nil {
+		panic(err)
+	}
+	var tpl bytes.Buffer
+	err = tmpl.Execute(&tpl, templateData)
+	if err != nil {
+		panic(err)
+	}
+	return tpl.String()
+}