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

Find operating system for deploy regardless of juju controller version. #358

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Find operating system for deploy regardless of juju controller version. #358

merged 3 commits into from
Dec 7, 2023

Conversation

hmlanigan
Copy link
Member

@hmlanigan hmlanigan commented Dec 5, 2023 •
edited
Loading

Description

Ensure we always have a suggested series from resolve charm. The value will be in different places depending on the juju controller version. Prefer the base value in the resolved charm origin. Fallback to the series from resolved charm.URL.

Updated the canary, add-machine, and integration test workflows to test with a 3.1.x controller as well as 2.9. Help find issues like this before release. The default microk8s snap channel provides a classic snap version. 3.x juju requires a strictly confined version of the microk8s snap. Added channel to ensure both set of tests were using the same version of microk8s. We will have to update the versions in the future.

Updated a test due to changes in the charm used by the test.

Fixes: #352

Type of change

  • Change in tests (one or several tests have been changed)
  • Bug fix (non-breaking change which fixes an issue)

Environment

  • Juju controller version: 3.1.6

  • Terraform version: 1.6.5

QA steps

Run these steps against juju 2.9/stable and 3.1/stable juju controllers.

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

provider "juju" {
}

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

resource "juju_application" "kubernetes-control-plane" {
  model = juju_model.testmodel.name
  charm {
    name     = "kubernetes-control-plane"
    base     = "ubuntu@22.04"
    channel  = "1.28/stable"
  }
}
terraform init  && terraform plan && terraform apply

Additional notes

JUJU-5169
JUJU-4544

@hmlanigan hmlanigan added this to the 0.10.1 milestone Dec 5, 2023
@hmlanigan hmlanigan force-pushed the fix-issue-352 branch 2 times, most recently from 07cad8f to 33718e7 Compare December 5, 2023 21:09
@hmlanigan hmlanigan changed the title WIP: Find operating system for deploy regardless of juju controller version. Find operating system for deploy regardless of juju controller version. Dec 6, 2023
@hmlanigan
Ensure we have a suggested series from resolveCharm.
5418829 
Between juju 2.9 and 3.x, there is a move from using series to base.
This provider client is using the juju 2.9.46 code base for API, but may
be run against 2.9, 3.1.x, 3.3.x controller with the expection that
current functionality will work. Trust the base first as 3.1 may not
return a series in the charm.URL. Fall back is the series from the URL.
@hmlanigan
Include juju 3.1/stable when running canary and integration tests.
ec94489 
Should have caught issue juju#352 earlier. Increase our testing coverage to
help fill the gap.

Split into 2 test matrices for juju 2.9 & 3.1

Juju 3.1 requires a strictly confined version of the microk8s snap, will
2.9 uses classic. Further splitting of the tests required due to bug in
charmed-kubernetes actions operator, there is not a tag specific for a
microk8s channel.
@hmlanigan
Use matrix include.
4fdf3e4 
This solves the problem in the matrix where the channel is only
necessary for microk8s clouds and not lxd. It also allows for 1 test
configuration rather than many..
@hmlanigan
Copy link
Member Author

rebased after landing a pr also fixing the offer upgrade test

anvial
anvial reviewed Dec 7, 2023
View reviewed changes
.github/workflows/canary.yml
- "1.6.*"
include:
Copy link
Member

Choose a reason for hiding this comment

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

nice solution.

anvial
anvial approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@anvial anvial left a comment

Choose a reason for hiding this comment

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

Looks good,

tested on 2.9 and 3.1 controllers.

@hmlanigan hmlanigan merged commit 2e7093f into juju:main Dec 7, 2023
14 checks passed
@hmlanigan hmlanigan deleted the fix-issue-352 branch December 7, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anvial anvial anvial approved these changes

@cderici cderici Awaiting requested review from cderici

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10.1
2 participants
@hmlanigan @anvial