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.

Sign up for GitHub

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

Jump to bottom

feat(storage): add storage support to application resource #494

Merged
merged 4 commits into from
Jul 3, 2024
Merged

feat(storage): add storage support to application resource #494

merged 4 commits into from
Jul 3, 2024

Conversation

anvial
Copy link
Member

@anvial anvial commented Jun 4, 2024 •
edited by hmlanigan
Loading

Description

This PR adds storage support for application resources.

Fixes: #198

Type of change

  • Change existing resource

QA steps

terraform {
  required_providers {
    juju = {
      source  = "juju/juju"
      version = "0.12.0"
    }
  }
}
provider "juju" {}

resource "juju_model" "storage-model" {
  name = "storage-model"
}

resource "juju_application" "postgresql" {
  name  = "postgresql"
  model = juju_model.storage-model.name

  charm {
    name     = "postgresql"
    channel  = "latest/stable"
  }

  storage_directives = { pgdata = "8M" }

  units = 1
}
...

@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch from d46ef9b to f45ec42 Compare June 5, 2024 19:32
@anvial anvial changed the title [WIP] Add storage support to application resource. [WIP] feat(storage): Add storage support to application resource. Jun 6, 2024
@anvial anvial changed the title [WIP] feat(storage): Add storage support to application resource. [WIP] feat(storage): Add storage support to application resource Jun 6, 2024
@hmlanigan hmlanigan added this to the 0.13.0 milestone Jun 6, 2024
@hmlanigan hmlanigan self-requested a review June 6, 2024 12:38
@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch 4 times, most recently from b143fb3 to a96ec81 Compare June 6, 2024 20:50
@anvial anvial changed the title [WIP] feat(storage): Add storage support to application resource feat(storage): Add storage support to application resource Jun 6, 2024
@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch from a96ec81 to 4c1b081 Compare June 6, 2024 21:08
@anvial anvial changed the title feat(storage): Add storage support to application resource feat(storage): add storage support to application resource Jun 7, 2024
@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch 8 times, most recently from f19ed91 to 6031f4c Compare June 14, 2024 09:10
@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch 2 times, most recently from fc3a919 to 36007c5 Compare June 14, 2024 14:56
hmlanigan
hmlanigan reviewed Jun 23, 2024
View reviewed changes
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.

Here is my initial review. I haven't done QA yet. There is a conflict and perhaps some tests to fix as well.

go.mod Outdated Show resolved Hide resolved
internal/juju/applications.go Outdated Show resolved Hide resolved
internal/juju/applications.go Outdated Show resolved Hide resolved
internal/juju/applications.go Show resolved Hide resolved
internal/juju/applications.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 Show resolved Hide resolved
internal/provider/resource_application.go Outdated Show resolved Hide resolved
@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch from d014bb8 to af585a1 Compare June 24, 2024 16:31
@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch 5 times, most recently from 04cfe44 to b635d36 Compare July 1, 2024 16:42
@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch 4 times, most recently from 8bbf18c to a8f18d8 Compare July 2, 2024 12:44
@anvial
feat(application) update CRUD function for application resource
c27e1ac 
feat(resource_application): update CRUD functions

This commit updates the Create, Read, Update, and Delete (CRUD) functions in the `resource_application.go` file
to support defining storage for application resource.
It also includes changes in the corresponding test file `resource_application_test.go` to add additional test with
storage.
The changes aim to improve the functionality of working with storage for the application resource in the Juju Terraform provider.

The changes include:
- Improved error handling in the ReadApplicationWithRetryOnNotFound function.
- Added error handling for application not found in `handleApplicationNotFoundError` function.
- Added storage directives to Create and Read functions
- Modifier retry process to read application changes, to be able to wait
  for storage creation

Storage use comupeted struct that is used to stro information about
storage into the terraform state. Storage_directives is a special struck
that help to manupolate storage in application resource in terraform
plan.

test(application): add acceptance test for resource application storage

feat: introduce storage_directives map instead storage set

docs: modify placement example for resource_application to include
storage
@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch from a8f18d8 to c27e1ac Compare July 2, 2024 12:51
@anvial anvial requested a review from hmlanigan July 2, 2024 15:51
@hmlanigan
fix(storage): update to work on juju 2.9
2f08994 
The test on 2.9 was failing because the Deploy arguments in legacyDeploy
did not include storage constraints. Updated the test as well to ensure
it works with 2.9 and 3.x by checking all values.
@hmlanigan hmlanigan force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch from 306648c to 2f08994 Compare July 2, 2024 19:55
@hmlanigan
refactor: remove use of Private data key: storage
3c59d4b 
It is no longer needed with the switch from using just storage to to
using storage and storage_directives in the schema as we can write
storage without impact to the plan supplied directives.
hmlanigan
hmlanigan reviewed Jul 2, 2024
View reviewed changes
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.

I added 2 commits, one to enable storage for juju controllers not using DeployFromRepository and to remove the Private data usage.

Once the rest of these smaller comments are addressed, we should be good to land.

examples/resources/juju_application/resource.tf Outdated Show resolved Hide resolved
internal/juju/applications.go Show resolved Hide resolved
internal/juju/applications.go Outdated Show resolved Hide resolved
internal/juju/applications.go Outdated Show resolved Hide resolved
internal/juju/applications.go Outdated Show resolved Hide resolved
internal/provider/resource_application_test.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
Comment on lines 567 to 568
// trace storage constraints
r.trace("create application storage constraints", map[string]interface{}{"storageConstraints": storageConstraints})
Copy link
Member

Choose a reason for hiding this comment

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

delete me

@anvial anvial force-pushed the git-checkout--b-JUJU-6023-update-resource-crud-functions branch from 2822231 to 63b56da Compare July 3, 2024 08:59
@anvial anvial requested a review from hmlanigan July 3, 2024 09:56
hmlanigan
hmlanigan approved these changes Jul 3, 2024
View reviewed changes
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.

LGTM! Thank you!

@hmlanigan hmlanigan merged commit cd4ebd4 into juju:main Jul 3, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmlanigan hmlanigan hmlanigan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.

Add ability to manage Juju storages
2 participants
@anvial @hmlanigan