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

Populating Juju controller config no longer immediately fails if Juju CLI does not exist #362

Closed

Conversation

Osama-Kassem
Copy link
Contributor

Description

Populating Juju controller config will fail if the Juju CLI does not exist. This PR changes that behaviour to attempt to fill the config using environment variables. Only if environment variables are incomplete will the function error.

Fixes: #340.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Environment

  • Juju controller version: 2.9.47

  • Terraform version: v1.5.7

QA steps

Manual QA steps should be done to test this PR.

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

data "juju_model" "tf_test" {
  name = "tf-test"
}

resource "juju_application" "postgresql-k8s" {
  model = data.juju_model.tf_test.name
  charm {
    name = "postgresql-k8s"
  }
}
# Setup env
juju bootstrap microk8s terraform
juju add-model tf-test
terraform init

# This will get config through Juju CLI
terraform apply

export JUJU_CONTROLLER_ADDRESSES="$(juju show-controller | yq '.[$CONTROLLER]'.details.\"api-endpoints\" | tr -d "[]' "|tr -d '"'|tr -d '\n')"
export JUJU_USERNAME="$(cat ~/.local/share/juju/accounts.yaml | yq .controllers.$CONTROLLER.user|tr -d '"')"
export JUJU_PASSWORD="$(cat ~/.local/share/juju/accounts.yaml | yq .controllers.$CONTROLLER.password|tr -d '"')"
export JUJU_CA_CERT="$(juju show-controller $(echo $CONTROLLER|tr -d '"') | yq '.[$CONTROLLER]'.details.\"ca-cert\"|tr -d '"'|sed 's/\\n/\n/g')"

# Remove Juju config, to make Juju CLI error. You can try by doing `juju show-controller --show-password`
mv ~/.local/share/juju ~/.local/share/juju.backup
terraform apply # Should still work by fetching controller data from the env only

# Revert backup and run Juju status
rm -rf ~/.local/share/juju && mv ~/.local/share/juju.backup/ ~/.local/share/juju
juju status

@hmlanigan hmlanigan requested review from hmlanigan and cderici and removed request for hmlanigan December 7, 2023 13:58
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.

Initial comments - @cderici will assist moving forward

internal/provider/provider.go Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
@Osama-Kassem Osama-Kassem force-pushed the fix-juju-cli-couldnt-be-accessed branch from 0940c0e to f55b608 Compare December 8, 2023 11:42
internal/provider/provider.go Outdated Show resolved Hide resolved
@Osama-Kassem Osama-Kassem force-pushed the fix-juju-cli-couldnt-be-accessed branch from f55b608 to a7a54e2 Compare December 13, 2023 20:50
Copy link

@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.

Thanks for the changes @Osama-Kassem! This LGTM. There's still a scenario where this fails if we want partial values to be passed in via the plan + the env vars together, but we'll keep this PR scoped as you kindly wrote in the description, and put up another one to streamline those edge cases shortly after this 👍 Thanks again for all the efforts, feel free to fire up more PRs! :)

@cderici
Copy link

cderici commented Dec 13, 2023

@Osama-Kassem Our repo policy requires all the commits to be signed before it can be merged. Just to make things faster, we'll cherry-pick this commit you have here into the new PR I talked about earlier and sign it ourselves to get these changes into the new release tomorrow 👍 Thanks again for your work!

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.

terraform apply fails if there are no registered controllers
3 participants