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

feat: jaas group data source #602

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pkulik0
Copy link
Contributor

@pkulik0 pkulik0 commented Oct 3, 2024

Description

This PR adds a data source for JAAS groups. Additionally it removes unused arguments from jaas client methods and upgrades the version of jimm-go-sdk.

Issue: #604

Type of change

  • Add new resource
  • Other - changes to jaas client

QA steps

Run this against a JAAS controller:

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

resource "juju_jaas_group" "test" {
  name = "group-0"
}

data "juju_jaas_group" "test" {
  name = juju_jaas_group.test.name
}

output "group_uuid" {
  value = data.juju_jaas_group.test.uuid
}

@jujubot
Copy link
Contributor

jujubot commented Oct 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented Oct 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

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.

This PR is missing documentation for the new data source, QA steps in the PR, and commit message does not cover what was done and any.

It should be done in at least 2 commits, rather than including the version bump into other changes.

@hmlanigan hmlanigan added the state/codefreeze this pr cannot land until the code freeze has lifted. label Oct 4, 2024
@pkulik0
Copy link
Contributor Author

pkulik0 commented Oct 9, 2024

Please re-run the failed tests when you're here. They failed because of a TLS handshake time out when downloading a file.

Copy link
Contributor

Choose a reason for hiding this comment

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

One suggestion, add an example in examples/data-sources/juju_jaas_group/ and then regenerate the docs and it will show up in here.
That's thanks to the way TF generates docs, you can see custom templates in templates/

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.

One comment, and please update to include the example per @kian99.

We're having some issues with the terraform registry which will delay landing this PR.

"github.com/hashicorp/terraform-plugin-framework/datasource/schema"
"github.com/hashicorp/terraform-plugin-framework/types"
"github.com/hashicorp/terraform-plugin-log/tflog"
"github.com/juju/terraform-provider-juju/internal/juju"
Copy link
Member

Choose a reason for hiding this comment

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

todo: add newline above. This group of imports should be in 3 stanzas.

@kian99
Copy link
Contributor

kian99 commented Nov 6, 2024

@hmlanigan
Can we look at landing this PR?
It's missing the example for the documentation and the imports need to be split.
I can make a follow-up after this lands to get that in. Otherwise I was trying to make the changes directly into Piotr's branch but it doesn't seem possible since the PR doesn't allow repo maintainers to make edits.

@kian99 kian99 requested a review from hmlanigan November 6, 2024 10:29
@hmlanigan
Copy link
Member

I'm holding off landing this PR as we wanted to verify the release process by releasing a patch. I have work to update some documents in process to make the patch useful. We'd rather test the release process with a patch which has no schedule. We're currently using only one branch which necessitates the hold. This change requires a minor version release.

Nor could it land today with the failing required tests. We need to kick of the tests again.

@kian99
Copy link
Contributor

kian99 commented Nov 22, 2024

@hmlanigan Any update on getting this in?

@kian99
Copy link
Contributor

kian99 commented Nov 22, 2024

I've added the example and updated the imports as requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/codefreeze this pr cannot land until the code freeze has lifted.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants