From cb05e4e947145b8508278235aee75e510137ee55 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 27 Feb 2024 16:12:03 -0500 Subject: [PATCH 1/3] Do not update charm revision and channel at one time. The provider has no way to determine which takes precendence. --- internal/provider/resource_application.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index b5ed0b76..75e1a242 100644 --- a/internal/provider/resource_application.go +++ b/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) { From ef44853535d5a04c7088b9f6d7852c3909f248b7 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 27 Feb 2024 16:13:15 -0500 Subject: [PATCH 2/3] Fix update by revision and add more checks. Mold the origin used for charm update if a new revision is specified by the user. Details in comments. Otherwise juju will find the latest revision in the channel to use. Also added checks to ensure that the requested revision supports the architecture and operating system currently deployed. --- internal/juju/applications.go | 47 +++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/internal/juju/applications.go b/internal/juju/applications.go index b77895ec..75ad1d09 100644 --- a/internal/juju/applications.go +++ b/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, } From 3d3d4a38706bf57a3554632edd9e4c76a8e6d484 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Wed, 21 Feb 2024 13:06:47 +0300 Subject: [PATCH 3/3] Moves plan generator from `utils` package to `testing`. Cherry picked commit, fixed typo in directory name and removed config change from test as it won't work here. Will be added later. --- .../provider/resource_application_test.go | 67 +++++++++++++++ internal/testing/plangenerator.go | 84 +++++++++++++++++++ 2 files changed, 151 insertions(+) create mode 100644 internal/testing/plangenerator.go diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go index b0b93488..867b688f 100644 --- a/internal/provider/resource_application_test.go +++ b/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 { diff --git a/internal/testing/plangenerator.go b/internal/testing/plangenerator.go new file mode 100644 index 00000000..4ddf99d0 --- /dev/null +++ b/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() +}