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

Fixes Config/Revision update ordering. #407

Conversation

anvial
Copy link
Member

@anvial anvial commented Feb 15, 2024

Description

This PR puts a Revision update before a Config update in the update plan function of the plugin. This will help to avoid issues that prevent working with new config parameters, which are added in new charm revisions.

Fixes:

#389

Type of change

  • Logic changes in resources (the API interaction with Juju has been changed)
  • (WIP) Change in tests (one or several tests have been changed)

Environment

  • Juju controller version:
    3.3.1

  • Terraform version:
    1.7.3

QA steps

Manual QA steps should be done to test this PR.

juju boostrap microk8s k8s
juju add-model test

Apply plan:

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

resource "juju_application" "githubrunner" {
  name  = "github-runner"
  model = "test"


  charm {
    name     = "github-runner"
    revision = 8
    channel  = "latest/edge"
  }

  units = 1
}

Then update plan to

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

resource "juju_application" "githubrunner" {
  name  = "github-runner"
  model = "test"


  charm {
    name     = "github-runner"
    revision = 10
    channel  = "latest/edge"
  }

  config = {
    group = "mygroup"
   }

  units = 1
}

@anvial anvial added this to the 0.11.0 milestone Feb 15, 2024
@anvial anvial force-pushed the JUJU-5450-ordering-of-charm-upgrade-and-config-change branch from ec134b1 to 99cb5b3 Compare February 15, 2024 09:33
@anvial anvial changed the title [WIP] Fixes Config/Revision update ordering. Fixes Config/Revision update ordering. Feb 20, 2024
@anvial
Copy link
Member Author

anvial commented Feb 20, 2024

@hmlanigan, looks like we have problems with the acceptance tests regardless to this PR. Need to discuss with you of how to fix them.

@hmlanigan
Copy link
Member

@anvial If you've rebased this week, some 2.9 tests will fail because juju 2.9.47 hasn't been released yet (see #405 ). Failures against juju 3.x should be investigated.

@hmlanigan
Copy link
Member

@anvial, the plans in the qa steps should either create the model, or make it a data source if you run juju add-model ahead. While the given plan will work, it's not a good TF form as it doesn't reference the model.

@anvial
Copy link
Member Author

anvial commented Feb 20, 2024

@anvial, the plans in the qa steps should either create the model, or make it a data source if you run juju add-model ahead. While the given plan will work, it's not a good TF form as it doesn't reference the model.

But I thought that I create a model but this resource definition in plan:

resource "juju_model" "this" {
  name = "{{.ModelName}}"
}
...

Where modelName is generated for every test.

@hmlanigan
Copy link
Member

But I thought that I create a model but this resource definition in plan:

@anvial I'm speaking of the QA steps in the PR description, not the ACC test you've written for the resource_application_test.go file.

@anvial
Copy link
Member Author

anvial commented Feb 20, 2024

But I thought that I create a model but this resource definition in plan:

@anvial I'm speaking of the QA steps in the PR description, not the ACC test you've written for the resource_application_test.go file.

Agreed,

Now fixed in the QA steps description also, but it looks like it should be fixed in both places because of the chosen charm for the test: "github-runner".

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.

QA was good with revisions 88 and 95.

Please make the test changes and let's squash as many commits as we can? Looks like 2 commits are sufficient.

Lastly the QA steps listed in the PR description need updates to handle the model.

internal/juju/applications.go Outdated Show resolved Hide resolved
internal/provider/resource_application_test.go Outdated Show resolved Hide resolved
internal/provider/resource_application_test.go Outdated Show resolved Hide resolved
internal/provider/resource_application_test.go Outdated Show resolved Hide resolved
internal/utils/plangenerator.go Outdated Show resolved Hide resolved
@anvial anvial force-pushed the JUJU-5450-ordering-of-charm-upgrade-and-config-change branch 10 times, most recently from 0561709 to f5ed7ad Compare February 26, 2024 09:09
@anvial anvial force-pushed the JUJU-5450-ordering-of-charm-upgrade-and-config-change branch 3 times, most recently from 5bc2d27 to 1e7473a Compare February 28, 2024 18:44
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.

Just a note that a file has been duplicated and needs removal. Otherwise looks good.

internal/testsing/plangenerator.go Outdated Show resolved Hide resolved
@anvial anvial force-pushed the JUJU-5450-ordering-of-charm-upgrade-and-config-change branch 2 times, most recently from c8fef5c to 1a295f1 Compare February 28, 2024 19:12
This PR puts Revision update before Config update in update plan
function of the plugin. This will help to avoid issues that prevernt
working with new config parames which are added in new charm revisions.

Adds unit-test based on text/template.

Moves GetStringFromTemplateWithData to utils package.

Changes revision in unit-tests.

Skips whole unit-test on LXD instead of skipping each step.
@anvial anvial force-pushed the JUJU-5450-ordering-of-charm-upgrade-and-config-change branch from 1a295f1 to fa8703b Compare March 1, 2024 08:01
@anvial anvial merged commit f91e9e3 into juju:main Mar 1, 2024
16 of 23 checks passed
@cderici cderici mentioned this pull request Mar 18, 2024
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