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: add jaas client #540

Merged
merged 9 commits into from
Aug 20, 2024
Merged

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Aug 6, 2024

Description

This PR introduces the JAAS client to the provider. A full list of changes:

  • Introduced a dependency on jimm-go-sdk module that provides the client.
    • The jimm-go-sdk currently has a dependency on Juju 3.5.3 which Go bumps to when running go get github.com/canonical/jimm-go-sdk/v3
  • Introduced a jaas.go file in internal/juju which contains the client code. The only methods are a read/add/delete relations.
  • Introduced mocked tests in internal/juju/jaas_test.go which uses GoMock like the tests for secrets.go.

I recommend reviewing per PR to view the implementation and tests separately.

Type of change

  • Other: Adds new client to internal/juju/.

Additional notes

Resolves https://warthogs.atlassian.net/browse/CSS-9773

@kian99 kian99 force-pushed the CSS-9773-add-jaas-client branch from 7c97c25 to 5e8e75e Compare August 6, 2024 09:55
internal/juju/interfaces.go Show resolved Hide resolved
internal/juju/jaas.go Outdated Show resolved Hide resolved
internal/juju/jaas.go Outdated Show resolved Hide resolved
internal/juju/jaas.go Outdated Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
internal/juju/jaas.go Outdated Show resolved Hide resolved
internal/juju/jaas_test.go Outdated Show resolved Hide resolved
internal/juju/jaas_test.go Outdated Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
@hmlanigan hmlanigan self-requested a review August 6, 2024 12:56
go.mod Outdated Show resolved Hide resolved
internal/juju/jaas.go Outdated Show resolved Hide resolved
internal/juju/offers.go Show resolved Hide resolved
internal/juju/common_test.go Outdated Show resolved Hide resolved
internal/juju/jaas.go Show resolved Hide resolved
internal/juju/jaas_test.go Outdated Show resolved Hide resolved
internal/juju/jaas_test.go Outdated Show resolved Hide resolved
internal/juju/jaas_test.go Outdated Show resolved Hide resolved
internal/juju/jaas_test.go Outdated Show resolved Hide resolved
internal/juju/jaas.go Outdated Show resolved Hide resolved
@kian99 kian99 requested a review from hmlanigan August 7, 2024 08:50
@kian99 kian99 force-pushed the CSS-9773-add-jaas-client branch from 36a5d48 to d2dd485 Compare August 7, 2024 08:51
@kian99 kian99 force-pushed the CSS-9773-add-jaas-client branch 2 times, most recently from 4d78efb to dc25c74 Compare August 15, 2024 08:18
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.

With one nit, I'm happy with the changes.

@alesstimec are you happy with the changes?

internal/juju/jaas_test.go Outdated Show resolved Hide resolved
Copy link
Member

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

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

LGTM with a nitpick/suggestion


type jaasClient struct {
SharedClient
getJaasApiClient func(jujuapi.Connection) JaasAPIClient
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: we could call this newAPIClient.. we know it's a JAAS api client as it is a method of the jaasClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's at least in line with the naming style used for the other clients, but I see your point.

@kian99 kian99 force-pushed the CSS-9773-add-jaas-client branch from e356153 to 960de19 Compare August 20, 2024 06:50
@hmlanigan hmlanigan merged commit 72bb72d into juju:jaas-resources Aug 20, 2024
27 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.

3 participants