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

Add more configuration properties #80

Merged
merged 1 commit into from
Jan 2, 2025
Merged

Add more configuration properties #80

merged 1 commit into from
Jan 2, 2025

Conversation

dheid
Copy link

@dheid dheid commented Dec 20, 2024

This pull request introduces several changes to the README.md documentation and the Spring Boot auto-configuration for OpenFGA. The major changes include:

  • Extensive updates to the configuration properties.
  • Addition of telemetry configuration.
  • Improvements to the codebase structure.

Description

Documentation updates:

  • README.md: Reformatted the text for better readability and added a full example of the configuration in YAML format. Included detailed descriptions for each configuration property.
    • Reformatting and content updates for better readability and completeness.
    • Added a YAML configuration example and detailed property descriptions.

Spring Boot auto-configuration improvements:

  • OpenFgaAutoConfiguration.java: Refactored to use PropertyMapper for mapping configuration properties to ClientConfiguration. Added support for new properties such as userAgent, readTimeout, connectTimeout, maxRetries, minimumRetryDelay, defaultHeaders, and telemetryConfiguration.

    • Improved property mapping using PropertyMapper.
    • Added new configuration properties.
  • OpenFgaProperties.java: Added new configuration properties for user agent, timeouts, retries, default headers, and telemetry configuration.

    • Added new configuration properties.
    • Updated property handling logic.

Telemetry configuration:

  • TelemetryAttribute.java: Added an enum to represent telemetry attributes.

    • New enum for telemetry attributes.
  • TelemetryMetric.java: Added an enum to represent telemetry metrics.

    • New enum for telemetry metrics.

Testing improvements:

  • OpenFgaTest.java: Removed public modifiers from test methods to adhere to JUnit 5 conventions.
    • Adhered to JUnit 5 conventions by removing public modifiers from test methods.

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@dheid dheid requested a review from a team as a code owner December 20, 2024 10:09
@dheid
Copy link
Author

dheid commented Dec 20, 2024

@jimmyjames Could you look into this PR, please? I think, it is a great improvement.

@dheid dheid changed the title Add more configuration properties. Closes #70 Add more configuration properties Dec 20, 2024
@dheid
Copy link
Author

dheid commented Dec 20, 2024

@jimmyjames Another thing: Is it possible to use version 1.0.0. Some people think, that the OpenFGA Spring Boot Starter is not very mature because of the versioning that seems more like an alpha version.

@jimmyjames
Copy link
Collaborator

👋 Thanks for the PR @dheid! ❤️

I'm on holiday until the week of December 30th, so I will spend some time reviewing it when I'm back. Thanks for the understanding!

@dheid dheid force-pushed the main branch 5 times, most recently from b541943 to f6da4fd Compare December 23, 2024 10:32
@dheid
Copy link
Author

dheid commented Dec 31, 2024

👋 Thanks for the PR @dheid! ❤️

I'm on holiday until the week of December 30th, so I will spend some time reviewing it when I'm back. Thanks for the understanding!

Hey @jimmyjames ! Any news?

Copy link
Collaborator

@jimmyjames jimmyjames left a comment

Choose a reason for hiding this comment

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

@dheid this is looking great, awesome contribution! I left a couple questions in line, and also a couple other things:

  • can we document the new properties default values if not specified?
  • can we update the example application to use the new properties and telemetry config? Not only does that show how to use them for others, but also lets us do some testing on it.

build.gradle Outdated Show resolved Hide resolved
build.gradle Show resolved Hide resolved
@jimmyjames
Copy link
Collaborator

@jimmyjames Another thing: Is it possible to use version 1.0.0. Some people think, that the OpenFGA Spring Boot Starter is not very mature because of the versioning that seems more like an alpha version.

We will need to get to v1.0.0 of the java SDK first, since that is part of the starter's API. I can't provide any timeline for that unfortunately, but we do try and limit breaking changes and clearly document when we need to.

@dheid
Copy link
Author

dheid commented Jan 2, 2025

@jimmyjames @piotrooo I did the changes. That was a lot of work and I don't have too much time to do more work at the moment. I hope everything is sufficient now and we can merge this. Thanks a lot for your good review and bringing this forward!

@dheid dheid requested a review from jimmyjames January 2, 2025 11:26
@jimmyjames
Copy link
Collaborator

@jimmyjames @piotrooo I did the changes. That was a lot of work and I don't have too much time to do more work at the moment. I hope everything is sufficient now and we can merge this. Thanks a lot for your good review and bringing this forward!

It looks great! I'll give it another pass over but unless there's something unexpected, we'll get it merged and any other tweaks we (Okta) can do later. Really can't express how much we value this contribution, it's so appreciated! ❤️

@jimmyjames jimmyjames added this pull request to the merge queue Jan 2, 2025
Merged via the queue into openfga:main with commit aea68f9 Jan 2, 2025
7 checks passed
@jimmyjames jimmyjames mentioned this pull request Jan 6, 2025
4 tasks
github-merge-queue bot pushed a commit that referenced this pull request Jan 6, 2025
<!-- Thanks for opening a PR! Here are some quick tips:
If this is your first time contributing, [read our Contributing
Guidelines](https://github.com/openfga/.github/blob/main/CONTRIBUTING.md)
to learn how to create an acceptable PR for this repo.
By submitting a PR to this repository, you agree to the terms within the
[OpenFGA Code of
Conduct](https://github.com/openfga/.github/blob/main/CODE_OF_CONDUCT.md)

If your PR is under active development, please submit it as a "draft".
Once it's ready, open it up for review.
-->

<!-- Provide a brief summary of the changes -->

## Description
<!-- Provide a detailed description of the changes -->

- feat: add support for additional properties and support telemetry
(#80) - big shout out and thanks to @dheid for this!
- fix: use AutoConfiguration instead (#64) - thanks @eddumelendez!
- feat!: update OpenFGA Java version and spring version dependencies
(#74)

> [!WARNING]
> This version includes version 0.7.2 of the [OpenFGA Java
SDK](https://github.com/openfga/java-sdk/), which contains breaking
changes to the OpenFGA Java SDK. Please see #66 for additional
information.

## References
<!-- Provide a list of any applicable references here (GitHub Issue,
[OpenFGA RFC](https://github.com/openfga/rfcs), other PRs, etc..) -->

## Review Checklist
- [ ] I have clicked on ["allow edits by
maintainers"](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork).
- [ ] I have added documentation for new/changed functionality in this
PR or in a PR to [openfga.dev](https://github.com/openfga/openfga.dev)
[Provide a link to any relevant PRs in the references section above]
- [ ] The correct base branch is being used, if not `main`
- [ ] I have added tests to validate that the change in functionality is
working as expected
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