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

Create application resource in framework #265

Merged

Conversation

hmlanigan
Copy link
Member

@hmlanigan hmlanigan commented Aug 9, 2023

Note - there are 2 acceptance tests failing right now, though I'm not sure why. I cannot reproduce outside of the

Description

Convert the application resource from using the terraform sdk to the newer framework. There is a small bug fix related to exposing an application without an application name specified in the plan.

This PR also adds use the tflog package for logging including using a tflog subsystem for granularity. Each resource and data source should have its own subsystem. For applications, log line will include "@module":"juju.resource-application". To setup: export TF_LOG_PROVIDER=TRACE ; export TF_LOG_PATH=./terraform.log. Without the log path, logging will be printed within the normal terraform output.

Includes a fix for:
#274 Provider panics when generating plan

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Maintenance work (repository related, like Github actions, or revving the Go version, etc.)

Environment

  • Juju controller version: 2.9.43

  • Terraform version: 0.8.0 next

QA steps

Here is a plan to test with. Also do import testing.

terraform {
  required_providers {
    juju = {
      version = ">= 0.7.0"
      source  = "juju/juju"
    }
  }
}

provider "juju" {
}

resource "juju_model" "testmodel" {
  name = "machinetest"
}

resource "juju_application" "application_example" {
  model = juju_model.testmodel.name
  charm {
    name = "juju-qa-test"
  }
  units = 2
  expose {}
  config = {
	status = "application resource testing"
        foo-file = true
        skill = 42
  }
}

Notes

JUJU-4175
JUJU-4500

@hmlanigan hmlanigan requested a review from cderici August 9, 2023 20:00
@hmlanigan hmlanigan force-pushed the create-application-resource-in-framework branch 2 times, most recently from 49aaf93 to 3c4f944 Compare August 11, 2023 21:18
Copy link
Contributor

@cderici cderici left a comment

Choose a reason for hiding this comment

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

This looks great, actually, big piece of work 💪 just need some small adjustments, and the tests need to be fixed. The QA was mostly good, bumped into that no name specified error, import worked well 👍

internal/provider/resource_application.go Outdated Show resolved Hide resolved
internal/provider/resource_application.go Show resolved Hide resolved
internal/provider/resource_application.go Show resolved Hide resolved
internal/provider/resource_application.go Show resolved Hide resolved
internal/provider/resource_application.go Outdated Show resolved Hide resolved
internal/provider/resource_application.go Outdated Show resolved Hide resolved
internal/provider/resource_application.go Outdated Show resolved Hide resolved
internal/provider/resource_application.go Outdated Show resolved Hide resolved
internal/provider/resource_application.go Outdated Show resolved Hide resolved
@hmlanigan hmlanigan added this to the 0.9.0 milestone Aug 14, 2023
@hmlanigan hmlanigan force-pushed the create-application-resource-in-framework branch from 3c4f944 to 7a4ce21 Compare August 14, 2023 20:35
@hmlanigan
Copy link
Member Author

@cderici I pushed some some changes based on review comments, then rebased onto #269.

Copy link
Contributor

@cderici cderici left a comment

Choose a reason for hiding this comment

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

This is essentially ready to go, though please fix the tests before merge.

I just re-ran the tests because I'm not getting the same errors locally, not sure if CI is outdated. Anyways, the QA went well, however, I'm getting the following error when I run TestAcc_ResourceApplication_sdk2_framework_migrate locally.

resource_application_test.go:17: Step 6/6 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.

          map[string]string{
        -       "model": "tf-test-application-2356628633933833839",
        -       "name":  "test-app",
          }
--- FAIL: TestAcc_ResourceApplication_sdk2_framework_migrate (113.70s)

@hmlanigan
Copy link
Member Author

@cderici if the tests don't pass, we should be able to merge. and the button isn't green for me.

@hmlanigan hmlanigan force-pushed the create-application-resource-in-framework branch from 7a4ce21 to 70a3a27 Compare August 16, 2023 17:03
Move addClientNotConfiguredError there, create a intPtr method.
Use for generic helper methods within the provider.
With the TF framework, there is no longer a need to handle config as a
map[string]interface and do many convertions. Integers, strings and
booleans can all be added to the plan as strings, then converted by the
juju controller based on the actual config type defined by the charm.
@hmlanigan hmlanigan force-pushed the create-application-resource-in-framework branch from b06748e to fbd9af9 Compare August 16, 2023 18:34
@hmlanigan
Copy link
Member Author

force pushed to fix a merge conflict

There are no changes to the schema.

Introduce retry read application on not found error.

With the sdk, we could call the readApplication method from
createApplication method to fill in data only known after the
application is deploy such as is principal and placement. With the
framework this is not directly possible. Received application not found
errors.

The work around of setting values until the next read causes tests to
fail. Introduced a ReadApplicationWithRetryOnNotFound method to ensure
we get the application data if we know the application exists.

Make a map before using, avoid assignment to nil map panics.

Could happen if config is added, when it wasn't there before. Usually
appeared on k8s, not machines.
@hmlanigan hmlanigan force-pushed the create-application-resource-in-framework branch from a3d8ad4 to 6913c9e Compare August 17, 2023 18:54
@hmlanigan
Copy link
Member Author

squashed many of the commands for merging. the test failing due to a juju bug will be skipped for now and re-enabled when the bug is fixed.

@hmlanigan hmlanigan changed the title [JUJU-4175] Create application resource in framework Create application resource in framework Aug 22, 2023
With microk8s due to juju bug. Once the the juju bug is
fixed, it can be re-enabled.

With LXD for the same issue on microk8s where ReplaceRequired in schema
causing Delete/Create to happen too fast.
@hmlanigan hmlanigan force-pushed the create-application-resource-in-framework branch from 6913c9e to 8aaa02a Compare August 22, 2023 18:48
@hmlanigan hmlanigan merged commit e46f123 into juju:main Aug 22, 2023
2 checks passed
@hmlanigan hmlanigan deleted the create-application-resource-in-framework branch August 22, 2023 19:00
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.

2 participants