-
Notifications
You must be signed in to change notification settings - Fork 43
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
base: main
Are you sure you want to change the base?
Conversation
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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)
Linking to the referenced bug which the original comment doesn't seem to be doing - #521 |
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
QA steps
Manual QA steps should be done to test this PR.
Use juju status to see the application running. Then edit the plan to change storage_directive from
2G
to1G
and re-run the plan.Additional notes
JUJU-6348