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

Validate channel in application resource #447

Merged
merged 5 commits into from Apr 18, 2024

Conversation

Aflynn50
Copy link
Contributor

@Aflynn50 Aflynn50 commented Apr 11, 2024

Description

When specifying an application resource, the channel parameter specifies the channel on charmhub from which to fetch the charm. Right now, this is parsed by charm.Parse and anything it accepts is valid.

However, users have been running into issues where problems are caused due to the fact that their plan only specifies the risk, and not the track (#445). The default track is resolved by juju and errors like this are caused by the fact that juju has resolved the track and it is not present in the plan.

Rather than fixing the particular issues caused by these inconstancy, it makes more sense to for the users to specify themselves the actual track that they need, rather than relying on juju to resolve it. By requiring this, we make the juju terraform plans more repeatable (the default track could change between applications of a plan, meaning you can get different states for the same plan). It also removes this class of bugs.

It is still possible to omit a channel entirely, this may be desirable for non production plans where a user wants to just quickly test something and not have to bother going to charmhub to look up the default track.

Fixes:
#445

Type of change

  • Change existing resource
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Environment

  • Juju controller version:

  • Terraform version:

QA steps

Manual QA steps should be done to test this PR.

terraform {
  required_providers {
    juju = {
      version = "~> 0.11.0"
      source  = "juju/juju"
    }
  }
}

provider "juju" {}

resource "juju_model" "development" {
  name = "development"

  cloud {
    name   = "lxd"
  }
}

resource "juju_application" "wordpress" {
  name = "wordpress"

  model = juju_model.development.name

  charm {
    name = "ubuntu"
    base = "ubuntu@22.04"
    channel = "latest"
  }
}

On terraform plan this returns the error:

╷
│ Error: Invalid Channel
│ 
│   with juju_application.wordpress,
│   on plan.tf line 28, in resource "juju_application" "wordpress":
│   28:     channel = "latest"
│ 
│ String must conform to track/risk or track/risk/branch, e.g. latest/stable

When the channel is changed to "latest/stable" then terraform plan & terraform apply works to deploy the model

Additional notes

JUJU-5858

@Aflynn50 Aflynn50 changed the title Channel validator Validate channel in application resource Apr 11, 2024
@hmlanigan hmlanigan self-requested a review April 11, 2024 13:10
@Aflynn50 Aflynn50 marked this pull request as draft April 11, 2024 14:09
@Aflynn50 Aflynn50 marked this pull request as ready for review April 15, 2024 08:41
@hmlanigan hmlanigan added this to the 0.12.0 milestone Apr 15, 2024
@Aflynn50 Aflynn50 requested a review from cderici April 17, 2024 14:10
Copy link
Member

@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 LGTM, QA went well. Tried it with a bunch of different channel values (including ""), and the validation catches it during the plan as expected, nice work 👍

I think we should also have some unit tests for this, see internal application tests.

Makefile Outdated
JUJU_CONTROLLER_ADDRESSES=${CONTROLLER_ADDRESSES} \
JUJU_USERNAME=${USERNAME} JUJU_PASSWORD=${PASSWORD} \
JUJU_CA_CERT=${CA_CERT} TF_ACC=1 TEST_CLOUD=lxd go test ./... -v $(TESTARGS) -timeout 120m
JUJU_CONTROLLER_ADDRESSES=${CONTROLLER_ADDRESSES} JUJU_USERNAME=${USERNAME} JUJU_PASSWORD=${PASSWORD} JUJU_CA_CERT=${CA_CERT} TF_ACC=1 TEST_CLOUD=lxd go test ./... -v $(TESTARGS) -timeout 120m
Copy link
Member

Choose a reason for hiding this comment

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

Looks like your editor consolidated these into one line, I'd keep it on separate lines for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed back

@Aflynn50
Copy link
Contributor Author

This LGTM, QA went well. Tried it with a bunch of different channel values (including ""), and the validation catches it during the plan as expected, nice work 👍

I think we should also have some unit tests for this, see internal application tests.

I've added some unit tests for the validator

@Aflynn50 Aflynn50 merged commit 37d39c3 into juju:main Apr 18, 2024
25 checks passed
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.

None yet

3 participants