Update juju api version used to 3.3 #402

hmlanigan · 2024-02-09T22:13:30Z

Description

First updating the version of go used by this project to 1.21, which matches the version used by juju 3.3 branch.

Updates necessary with move to juju 3.3 api libraries.

Create a logger shim to allow the provider logger to be passed to and used by the juju api clients.
Update use of the juju/core/series package to the juju/core/base package. The series package has been removed.

Updated 3 of the GitHub workflows to replace juju 3.1 with 3.2. Once 3.3.2 is released, we can update to 3.3. 3.3.2 will fix the bug mentioned in the qa steps.

Type of change

Maintenance work (repository related, like Github actions, or revving the Go version, etc.)

Environment

Juju controller version: 2.9, 3.3
Terraform version: any

QA steps

Run the acceptance tests against a juju 2.9.47 controller and a 3.3/edge controller with no problem. You'll have to use the juju 2.9/edge snap to bootstrap. Due to this, some of the acceptance tests will have to be run manually and the GitHub action jobs will fail until juju 2.9.47 is released.

The failure in some 3.3 tests "Unable to create model, got error: interface conversion: interface {} is nil, not string" has been fixed by #16822 and is not yet in a released version of juju 3.3

Additional notes

JUJU-5121

Create a shim to map the terraform client logging to a juju Logger. This is in anticipation of the juju 3.3.0 changes to apiclient.NewClient, which will require a Logger.

Requires replacement of the core/series package use with core/base; a set of base helper methods to replace set.Strings funcationality; use of the new juju logger shim when calling apiclient.NewClient.

The provider is now using the 3.3 apis for juju. Let's test against that version and the oldest supported 2.9

Some of the places where setup-go was used, didn't specify a go version or where to find the version. After the update of go.mod to use go 1.21, some of the workflows started to fail due to the version of go. Avoid this in the future by adding go-version-file to the places it's missing.

Reduce files touched, was catching dependencies with go 1.21. Update to version 1.54.0

3.3.1 has a bug which will cause the tests to fail. Test against 3.2 until 3.3.2 has been released.

cderici

LGTM. Ignore the comments about the 3.3, didn't see your last commit in time, I was going through all the errors I was seeing, it might be worth to add a TODO in the code to change it after the 3.3.1 bug is resolved.

cderici · 2024-02-13T20:38:54Z

.github/workflows/test_add_machine.yml

@@ -52,7 +52,7 @@ jobs:
          - "1.6.*"
        juju:
          - "2.9/stable"
-          - "3.1/stable"
+          - "3.2/stable"


Suggested change

- "3.2/stable"

- "3.3/stable"

cderici · 2024-02-13T20:39:11Z

.github/workflows/canary.yml

@@ -45,10 +45,10 @@ jobs:
            juju-channel: "2.9/stable"
          - cloud: "lxd"
            cloud-channel: "5.19/stable"
-            juju-channel: "3.1/stable"
+            juju-channel: "3.2/stable"


Suggested change

juju-channel: "3.2/stable"

juju-channel: "3.3/stable"

cderici · 2024-02-13T20:39:24Z

.github/workflows/canary.yml

          - cloud: "microk8s"
            cloud-channel: "1.28-strict/stable"
-            juju-channel: "3.1/stable"
+            juju-channel: "3.2/stable"


Suggested change

juju-channel: "3.2/stable"

juju-channel: "3.3/stable"

cderici · 2024-02-13T20:40:03Z

.github/workflows/test_integration.yml

@@ -45,9 +48,9 @@ jobs:
        terraform: ["1.4.*", "1.5.*", "1.6.*"]
        action-operator:
          - { cloud: "lxd", cloud-channel: "5.19", juju: "2.9" }
-          - { cloud: "lxd", cloud-channel: "5.19", juju: "3.1" }
+          - { cloud: "lxd", cloud-channel: "5.19", juju: "3.2" }


Suggested change

- { cloud: "lxd", cloud-channel: "5.19", juju: "3.2" }

- { cloud: "lxd", cloud-channel: "5.19", juju: "3.3" }

cderici · 2024-02-13T20:40:11Z

.github/workflows/test_integration.yml

          - { cloud: "microk8s", cloud-channel: "1.28", juju: "2.9" }
-          - { cloud: "microk8s", cloud-channel: "1.28-strict", juju: "3.1" }
+          - { cloud: "microk8s", cloud-channel: "1.28-strict", juju: "3.2" }


Suggested change

- { cloud: "microk8s", cloud-channel: "1.28-strict", juju: "3.2" }

- { cloud: "microk8s", cloud-channel: "1.28-strict", juju: "3.3" }

cderici · 2024-02-13T20:53:50Z

internal/juju/applications.go

+		if err != nil {
+			return nil, err
+		}
+		urlForOrigin = urlForOrigin.WithSeries(userSuppliedSeries)


It might be worth checking what happens when a revision is specified, where the value of urlForOrigin will be overwritten by this line if a base is provided.

The WithSeries method of the URL will not overwrite the updates made by using WithRevision. The two With methods are updating independent pieces of the url and will not conflict.

cderici · 2024-02-13T21:18:09Z

internal/juju/applications.go

 // a private object.
-func (c applicationsClient) seriesToUse(modelconfigAPIClient *apimodelconfig.Client, inputSeries, suggestedSeries string, charmSeries set.Strings) (string, error) {
-	c.Tracef("seriesToUse", map[string]interface{}{"inputSeries": inputSeries, "suggestedSeries": suggestedSeries, "charmSeries": charmSeries.Values()})
+func (c applicationsClient) baseToUse(modelconfigAPIClient *apimodelconfig.Client, inputBase, suggestedBase base.Base, charmBases []base.Base) (base.Base, error) {


It is strange to me that we don't have unit tests for these functions, though that's beyond the scope of this PR, just wanted to note. I know that stuff we use from upstream (e.g. WorkloadBases(), ParseBaseFromString()) are tested there, but we do have some logic here that's covering a bunch of different scenarios, I feel like we should have unit tests for them (e.g. intersectionOfBases, basesContain since it uses IsCompatible).

Not just in this change, I feel like we should have unit tests for anything that starts with a lowercase letter in the internal/juju package.

I'm working on some unit tests for ReadApplicationWithRetryOnNotFound to help fix a different problem right now. However it'll be a bit trying to get more complete coverage. It's something we can chip away at for sure.

hmlanigan · 2024-02-14T21:21:53Z

@cderici I'll put a PR to set the tests to 3.3 - we can remember to merge it once we can get successful tests, i.e. 3.3.2 exists.

hmlanigan added this to the 0.11.0 milestone Feb 9, 2024

hmlanigan added 2 commits February 12, 2024 14:24

Update go version to 1.21, keeping inline with juju.

a46c15c

Add JujuLogger to SharedClient.

626fd21

Create a shim to map the terraform client logging to a juju Logger. This is in anticipation of the juju 3.3.0 changes to apiclient.NewClient, which will require a Logger.

hmlanigan force-pushed the use-juju-3-3-new branch from 211fe5d to a0466fb Compare February 12, 2024 19:37

hmlanigan added 2 commits February 12, 2024 15:00

Update to using juju 3.3.0 api code.

bb09b0d

Requires replacement of the core/series package use with core/base; a set of base helper methods to replace set.Strings funcationality; use of the new juju logger shim when calling apiclient.NewClient.

Use juju 3.3 instead of 3.1 for tests.

70896f2

The provider is now using the 3.3 apis for juju. Let's test against that version and the oldest supported 2.9

hmlanigan force-pushed the use-juju-3-3-new branch from b37bf15 to 70896f2 Compare February 12, 2024 20:00

hmlanigan added 3 commits February 12, 2024 15:10

linter update: unnecessary use

8592cf1

golangci-lint updates.

be125b6

Reduce files touched, was catching dependencies with go 1.21. Update to version 1.54.0

hmlanigan requested a review from cderici February 12, 2024 20:51

Use 3.2 instead of 3.3.

19ef087

3.3.1 has a bug which will cause the tests to fail. Test against 3.2 until 3.3.2 has been released.

cderici approved these changes Feb 13, 2024

View reviewed changes

hmlanigan merged commit 3f7dc79 into juju:main Feb 14, 2024
16 of 23 checks passed

hmlanigan deleted the use-juju-3-3-new branch February 14, 2024 21:36

hmlanigan mentioned this pull request Feb 14, 2024

Test with juju 3.4. #405

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update juju api version used to 3.3 #402

Update juju api version used to 3.3 #402

hmlanigan commented Feb 9, 2024 •

edited

Loading

cderici left a comment

cderici Feb 13, 2024

cderici Feb 13, 2024

cderici Feb 13, 2024

cderici Feb 13, 2024

cderici Feb 13, 2024

cderici Feb 13, 2024

hmlanigan Feb 14, 2024

cderici Feb 13, 2024

hmlanigan Feb 14, 2024

hmlanigan commented Feb 14, 2024

	- { cloud: "lxd", cloud-channel: "5.19", juju: "3.2" }
	- { cloud: "lxd", cloud-channel: "5.19", juju: "3.3" }

	- { cloud: "microk8s", cloud-channel: "1.28-strict", juju: "3.2" }
	- { cloud: "microk8s", cloud-channel: "1.28-strict", juju: "3.3" }

Update juju api version used to 3.3 #402

Update juju api version used to 3.3 #402

Conversation

hmlanigan commented Feb 9, 2024 • edited Loading

Description

Type of change

Environment

QA steps

Additional notes

cderici left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmlanigan commented Feb 14, 2024

hmlanigan commented Feb 9, 2024 •

edited

Loading