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

Merged
merged 3 commits into from
Dec 3, 2024
Merged

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

@kian99 kian99 requested review from hmlanigan and kian99 October 3, 2024 18:15
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 pkulik0 force-pushed the data-source-jaas-group branch from 848eda6 to 3407668 Compare October 8, 2024 13:49
@pkulik0 pkulik0 force-pushed the data-source-jaas-group branch from 3407668 to c566fa0 Compare October 8, 2024 13:50
@pkulik0 pkulik0 requested a review from hmlanigan October 9, 2024 13:07
@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.

@alesstimec alesstimec removed the state/codefreeze this pr cannot land until the code freeze has lifted. label Dec 3, 2024
@hmlanigan
Copy link
Member

/merge

@jujubot jujubot merged commit c271cfd into juju:main Dec 3, 2024
24 of 30 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.

5 participants