-
Notifications
You must be signed in to change notification settings - Fork 44
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
Conversation
1 similar comment
There was a problem hiding this 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.
848eda6
to
3407668
Compare
3407668
to
c566fa0
Compare
Please re-run the failed tests when you're here. They failed because of a TLS handshake time out when downloading a file. |
docs/data-sources/jaas_group.md
Outdated
There was a problem hiding this comment.
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/
There was a problem hiding this 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" |
There was a problem hiding this comment.
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.
@hmlanigan |
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. |
@hmlanigan Any update on getting this in? |
I've added the example and updated the imports as requested. |
/merge |
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
QA steps
Run this against a JAAS controller: