feat(update-app-base): add support to update `base` for application charms #652

SimoneDutto · 2025-01-07T11:10:36Z

Description

This PR adds support to update the base in application charms by requiring a replace in case of a machine charm, and perform the upgrade in case of a k8s charm.

Fixes: re #635

Type of change

Add support to update base for application charms

QA Steps

LXD

terraform {
  required_providers {
    juju = {
      version = "~> 0.15.0"
      source  = "juju/juju"
    }
  }
}

provider "juju" {
  controller_addresses = "10.229.36.127:17070"

  username = "admin"
  password = <redacted>

  ca_certificate = file("ca.crt")
}

resource "juju_model" "this" {
  name = "test-model"
}

resource "juju_application" "this" {
  model = juju_model.this.name
  name  = "test-app"
  charm {
    name = "ubuntu"
    base = "[email protected]"
  }
}

terraform apply
juju status and you should see base=22.04

Now update the base

resource "juju_application" "this" {
  model = juju_model.this.name
  name  = "test-app"
  charm {
    name = "ubuntu"
    base = "[email protected]"
  }
}

terraform plan and you should see ~ base = "[email protected]" -> "[email protected]" # forces replacement
terraform apply
Now we have two scenarios:

happy (maybe just in the tests): the changes go though and you now have your new charm with the updated charm.
sad: terraform tries to recreate the application before it was destroyed and errors out. If you rerun it, you now have your updated charm.
juju status and you should see base=20.04

Microk8s

terraform {
  required_providers {
    juju = {
      version = "~> 0.15.0"
      source  = "juju/juju"
    }
  }
}

provider "juju" {
  controller_addresses = "10.152.183.78:17070"

  username = "admin"
  password = "<redacted>"

  ca_certificate = file("ca.crt")
}

resource "juju_model" "this" {
  name = "test-model"
}

resource "juju_application" "this" {
  model = juju_model.this.name
  name  = "test-app"
  charm {
    name    = "coredns"
    base    = "[email protected]"
    channel = "1.25/stable"
  }
}

terraform apply
juju show-application test-app -> check base ubuntu 22.04
Update plan

resource "juju_application" "this" {
  model = juju_model.this.name
  name  = "test-app"
  charm {
    name    = "coredns"
    base    = "[email protected]"
    channel = "1.25/stable"
  }
}

terraform plan -> check that the plan doesn't require replace.
terraform apply
juju show-application test-app -> check base ubuntu 20.04

internal/provider/resource_application.go

hmlanigan

We're on the right track, just a few details to iron out.

The commit message should include WHY were fixing the bug the way it's being fixed.

internal/juju/applications.go

internal/provider/modifer_applicationbaseupdate.go

hmlanigan · 2025-01-22T21:48:44Z

internal/juju/applications.go

+	} else if input.Base != "" {
+		base, err := corebase.ParseBaseFromString(input.Base)
+		if err != nil {
+			return nil, err
+		}
+		newOrigin.Base = base


comment: It may be okay, to not consider the revision and base together, in this if statement. When we resolve a charm by revision, the base is ignored by the upstream server.

internal/juju/applications.go

hmlanigan · 2025-01-22T21:51:21Z

internal/juju/applications.go

+	} else if input.Base != "" {
+		oldOrigin.Base = newOrigin.Base


issue: This means that the revision and the base cannot change at the same time. Given that every charm revision has only 1 base, that is a problem when refreshing by revision rather than channel.

Take a look at line 1401 as well:

if !basesContain(oldOrigin.Base, supportedBases)

It's validating that the new potential charm supports the old base - which will cause problems if the base is being changed.

i've moved base's ifs to a separate block.

Btw, the charmhub view is misleading because it seems that a revision can have multiple bases. (an example can be found here: https://charmhub.io/coredns).

i've done one more qa test on my machine:
(microk8s)
from

resource "juju_application" "this" { model = juju_model.this.name name = "test-app" charm { name = "coredns" base = "[email protected]" revision = 33 } }

to

resource "juju_application" "this" { model = juju_model.this.name name = "test-app" charm { name = "coredns" base = "[email protected]" revision = 145 } }

and now works.

hmlanigan · 2025-01-22T21:52:11Z

internal/provider/resource_application.go

@@ -590,6 +605,7 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq
 	plan.Placement = types.StringValue(readResp.Placement)
 	plan.Principal = types.BoolNull()
 	plan.ApplicationName = types.StringValue(createResp.AppName)
+	plan.ModelType = types.StringValue(modelType.String())


todo: Use StringValue(), String() is a quoted value.

https://github.com/juju/terraform-provider-juju/wiki/Developing#model-tf-secret-test-not-found

it doesn't have any ValueString. It's a struct from the juju lib, it only has a String() method

hmlanigan · 2025-01-22T21:56:47Z

internal/provider/resource_application.go

@@ -692,6 +708,12 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest
 	}
 	r.trace("read application", map[string]interface{}{"resource": appName, "response": response})

+	modelType, err := r.client.Applications.ModelType(modelName)


todo: While technically harmless, this value should really be included in the CreateApplicationResponse and ReadApplicationResponse structures, rather than making a second call in each place. We're trying to keep a cleaner distinction between what the provider has to know and juju or other details.

i've add to both structs ModelType

internal/provider/modifer_applicationbaseupdate.go

hmlanigan · 2025-01-22T22:00:16Z

internal/provider/modifer_applicationbaseupdate.go

+	if req.State.Raw.IsKnown() {
+		var state applicationResourceModel
+		diags := req.State.Get(ctx, &state)
+		if diags.HasError() {
+			resp.Diagnostics.Append(diags...)
+			return
+		}
+		modelType := state.ModelType.ValueString()
+		resp.RequiresReplace = modelType == model.IAAS.String()
+	}


comment: Checking IfKnown on all of state is good. Though you've asked it of ALL the saved state is known. Which maybe too broad a question? I'd ask if the specific element IsKnown instead.

i've now moved it to ask for the model type specifically.

…harms This PR adds support to update the base in application charms by requiring a replace in case of a machine charm, and perform the upgrade in case of a k8s charm. We add the model_type computed field on the application schema, and we use it to make a decision in the planmodifier RequiresReplaceIf. re juju#635

hmlanigan

This shaped up nicely.

Please let the code freeze end before merging.

SimoneDutto force-pushed the fix-change-base branch 2 times, most recently from cec578d to 19c361e Compare January 7, 2025 13:04

SimoneDutto requested a review from hmlanigan January 7, 2025 15:42

hmlanigan requested changes Jan 14, 2025

View reviewed changes

internal/provider/resource_application.go Show resolved Hide resolved

SimoneDutto requested a review from hmlanigan January 15, 2025 11:32

alesstimec added the state/codefreeze this pr cannot land until the code freeze has lifted. label Jan 20, 2025

SimoneDutto force-pushed the fix-change-base branch 2 times, most recently from 804ac33 to 5431e7c Compare January 22, 2025 14:43

hmlanigan reviewed Jan 22, 2025

View reviewed changes

SimoneDutto force-pushed the fix-change-base branch 2 times, most recently from e04c126 to 963a08e Compare January 23, 2025 10:55

SimoneDutto force-pushed the fix-change-base branch from 963a08e to c1e9afc Compare January 23, 2025 10:57

SimoneDutto requested review from hmlanigan and alesstimec January 23, 2025 10:57

hmlanigan approved these changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(update-app-base): add support to update `base` for application charms #652

feat(update-app-base): add support to update `base` for application charms #652

SimoneDutto commented Jan 7, 2025 •

edited

Loading

hmlanigan left a comment

hmlanigan Jan 22, 2025

hmlanigan Jan 22, 2025

SimoneDutto Jan 23, 2025

SimoneDutto Jan 23, 2025 •

edited

Loading

hmlanigan Jan 22, 2025

SimoneDutto Jan 23, 2025

hmlanigan Jan 22, 2025

SimoneDutto Jan 23, 2025

hmlanigan Jan 22, 2025

SimoneDutto Jan 23, 2025

hmlanigan left a comment

feat(update-app-base): add support to update base for application charms #652

Are you sure you want to change the base?

feat(update-app-base): add support to update base for application charms #652

Conversation

SimoneDutto commented Jan 7, 2025 • edited Loading

Description

Type of change

QA Steps

LXD

Microk8s

hmlanigan left a 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

SimoneDutto Jan 23, 2025 • 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

hmlanigan left a comment

Choose a reason for hiding this comment

feat(update-app-base): add support to update `base` for application charms #652

feat(update-app-base): add support to update `base` for application charms #652

SimoneDutto commented Jan 7, 2025 •

edited

Loading

SimoneDutto Jan 23, 2025 •

edited

Loading