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 - remove force destroy of application offers. #651

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alesstimec
Copy link
Member

Description

Removes force removal of application offers.

Fixes:

Type of change

  • Change existing resource

@alesstimec alesstimec requested a review from hmlanigan December 18, 2024 16:20
@alesstimec alesstimec force-pushed the fix-force-remove-offer branch from bd7ac67 to 2a13a5a Compare December 18, 2024 16:21
internal/juju/offers.go Outdated Show resolved Hide resolved
for ok := true; ok; ok = len(offer.Connections) > 0 {
//if we have been failing to destroy offer for 5 minutes then fail on destroy
if time.Now().After(end) {
return fmt.Errorf("offer %q has remaining connections", input.OfferURL)
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly add to the log the remaining connections, so a final user can debug it/file a more precise bug report

Copy link
Member

Choose a reason for hiding this comment

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

@SimoneDutto good idea, I suggest different output though. Not all users will understand connections.

RelationID would be ideal, but the provider doesn't deal in RelationIDs like the juju cli does.

Let's try offer.SourceModelUID and offer.Endpoint in the message. It should have the application:relation in the string. That should be enough info to find the relation causing the issue.

Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

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

todo: update the commit message and pr description to include why this is being done.

These failures:

=== CONT  TestAcc_ResourceIntegrationWithMultipleConsumers
    resource_integration_test.go:222: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: Client Error
        
        Unable to delete offer, got error: offer
        "admin/tf-test-integration-6969741932116347544.a" has remaining connections
--- FAIL: TestAcc_ResourceIntegrationWithMultipleConsumers (382.32s)

are directly related to this change. It's a racey test now.

for ok := true; ok; ok = len(offer.Connections) > 0 {
//if we have been failing to destroy offer for 5 minutes then fail on destroy
if time.Now().After(end) {
return fmt.Errorf("offer %q has remaining connections", input.OfferURL)
Copy link
Member

Choose a reason for hiding this comment

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

@SimoneDutto good idea, I suggest different output though. Not all users will understand connections.

RelationID would be ideal, but the provider doesn't deal in RelationIDs like the juju cli does.

Let's try offer.SourceModelUID and offer.Endpoint in the message. It should have the application:relation in the string. That should be enough info to find the relation causing the issue.

internal/juju/offers.go Outdated Show resolved Hide resolved
@alesstimec alesstimec force-pushed the fix-force-remove-offer branch 2 times, most recently from 56970eb to cfab456 Compare December 20, 2024 14:39
Removes force removal of application offers, but rather errors out if there are existing integrations
with the offer that must be removed first. This is a much cleaner way to destroy offers as the use
of the `force` flag may leave the Juju state with leftover artefacts.
@alesstimec alesstimec force-pushed the fix-force-remove-offer branch from cfab456 to b734312 Compare December 20, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants