Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(application) wait for application to be destroyed #524

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hmlanigan
Copy link
Member

Description

Wait for an application to actually be destroyed when calling Destroy(). This is an issue where terraform is calling Destroy() then Create() when the former completes. Juju may not have had time to fully destroy the application before we try to recreate it.

A bit of refactoring to put errors in the same place and use errors.Join() rather than a home grown version.

Stop using 2 versions of the juju/names package, only one should be.

Fixes: 521

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Other (please describe)

QA steps

Manual QA steps should be done to test this PR.

provider juju {}
resource "juju_model" "this" {
  name = "unit-test"
}

resource "juju_application" "githubrunner-29" {
  name  = "github-runner"
  model = juju_model.this.name

  charm {
    name     = "github-runner"
    channel  = "latest/edge"
    base = "[email protected]"
  }
  units = 1

  storage_directives = {
    runner = "2G"
  }
}

Use juju status to see the application running. Then edit the plan to change storage_directive from 2G to 1G and re-run the plan.

Additional notes

JUJU-6348

When destroying an application, wait for it to be destroyed before
returning. Issue juju#521 and maybe others. Otherwise terraform fails when
RequireReplace is set.
Replace ProcessErrorResults with errors.Join which does the same thing
for us.
Two versions of the juju/names pacakge were directly used. Luckily there
was no conflict. Move to the most recent.
@hmlanigan hmlanigan requested review from cderici and anvial July 11, 2024 22:05
@hmlanigan hmlanigan added this to the 0.13.0 milestone Jul 11, 2024
Copy link
Member

@anvial anvial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not check PR QA, just comments for the code.

@@ -22,7 +22,6 @@ require (
github.com/juju/cmd/v3 v3.0.14
github.com/juju/collections v1.0.4
github.com/juju/errors v1.0.0
github.com/juju/names/v4 v4.0.0-20220207005702-9c6532a52823
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we mistakenly started using names/v4 instead v5 somewhere. I think we need to use just v5

@@ -24,7 +24,7 @@ import (
"github.com/hashicorp/terraform-plugin-framework/types"
"github.com/hashicorp/terraform-plugin-log/tflog"
"github.com/juju/juju/core/constraints"
"github.com/juju/names/v4"
"github.com/juju/names/v5"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to make these changes in every resource. (v4 --> v5)

@hmlanigan hmlanigan removed this from the 0.13.0 milestone Jul 29, 2024
@hloeung
Copy link

hloeung commented Nov 25, 2024

Linking to the referenced bug which the original comment doesn't seem to be doing - #521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants