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 groups schema and CRUD #570

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Conversation

pkulik0
Copy link
Contributor

@pkulik0 pkulik0 commented Sep 10, 2024

Description

This PR implements a resource for jaas groups, updates the jaas client to include required functions and covers the added functionality with tests.

CSS-9770 / CSS-9772

Type of change

  • Add new resource
  • Upgrade the version of jimm-go-sdk, and jimm in jaas integration test

@jujubot
Copy link
Contributor

jujubot commented Sep 10, 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 Sep 10, 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
Copy link
Contributor

kian99 commented Sep 10, 2024

Just taking a brief looking at this,

  • Change the target branch to jaas-resources rather than main.
  • I suggest splitting up this PR to add the client code and tests in one PR then the crud in a separate PR.

@pkulik0 pkulik0 changed the base branch from main to jaas-resources September 10, 2024 18:39
internal/juju/jaas.go Outdated Show resolved Hide resolved
internal/provider/resource_jaas_group.go Outdated Show resolved Hide resolved
internal/provider/resource_jaas_group.go Outdated Show resolved Hide resolved
internal/provider/resource_jaas_group_test.go Outdated Show resolved Hide resolved
internal/provider/resource_jaas_group_test.go Outdated Show resolved Hide resolved
internal/provider/resource_jaas_group_test.go Outdated Show resolved Hide resolved
internal/provider/resource_jaas_group_test.go Outdated Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
internal/provider/resource_jaas_group.go Show resolved Hide resolved
internal/provider/resource_jaas_group.go Show resolved Hide resolved
}

// Read the group from JAAS
group, err := resource.client.Jaas.ReadGroup(ctx, state.UUID.ValueString())
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering how this will work out. It's unexpected to read based off of a computed value from the plan, rather than a value required to be set.

internal/provider/resource_jaas_group_test.go Outdated Show resolved Hide resolved
internal/provider/resource_jaas_group_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kian99 kian99 left a comment

Choose a reason for hiding this comment

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

Nice work!

internal/juju/client.go Outdated Show resolved Hide resolved
internal/provider/resource_jaas_group.go Outdated Show resolved Hide resolved
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.

started the tests running, please review for success.

2 small items to address, if the tests are successful I'll approve.

go.sum Outdated Show resolved Hide resolved
@hmlanigan
Copy link
Member

/merge

@jujubot jujubot merged commit 3fc973c into juju:jaas-resources Sep 18, 2024
28 of 29 checks passed
@pkulik0 pkulik0 deleted the jaas-groups branch September 19, 2024 00:46
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.

4 participants