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.

Sign up for GitHub

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

Jump to bottom

[NET-5346] Expose JWKCluster fields in jwt-provider config entry #2881

Merged
merged 1 commit into from
Sep 14, 2023
Merged

[NET-5346] Expose JWKCluster fields in jwt-provider config entry #2881

merged 1 commit into from
Sep 14, 2023

Conversation

roncodingenthusiast
Copy link
Contributor

@roncodingenthusiast roncodingenthusiast commented Aug 31, 2023 •
edited
Loading

Changes proposed in this PR:

How I've tested this PR:

  • Manually
  • Unit tests

How I expect reviewers to test this PR:

TODO:

  • Tests added
  • changelog entry

@roncodingenthusiast roncodingenthusiast added the backport/1.2.x This release branch is no longer active. label Aug 31, 2023
@roncodingenthusiast roncodingenthusiast changed the title WIP - [NET-5346] Expose JWKCluster fields in jwt-provider config entry [NET-5346] Expose JWKCluster fields in jwt-provider config entry Aug 31, 2023
@roncodingenthusiast roncodingenthusiast requested review from a team, skpratt and kisunji and removed request for a team August 31, 2023 16:16
@roncodingenthusiast roncodingenthusiast changed the title [NET-5346] Expose JWKCluster fields in jwt-provider config entry WIP [NET-5346] Expose JWKCluster fields in jwt-provider config entry Aug 31, 2023
@david-yu david-yu linked an issue Aug 31, 2023 that may be closed by this pull request
JWKSCluster missing from CRD handling #2772
Closed
@roncodingenthusiast roncodingenthusiast marked this pull request as ready for review September 1, 2023 02:51
@roncodingenthusiast roncodingenthusiast changed the title WIP [NET-5346] Expose JWKCluster fields in jwt-provider config entry [NET-5346] Expose JWKCluster fields in jwt-provider config entry Sep 1, 2023
@roncodingenthusiast roncodingenthusiast mentioned this pull request Sep 1, 2023
Closed
@roncodingenthusiast roncodingenthusiast force-pushed the NET-5346 branch 2 times, most recently from ae7f87d to e2e7ddc Compare September 5, 2023 19:33
kisunji
kisunji reviewed Sep 6, 2023
View reviewed changes
Copy link
Contributor

@kisunji kisunji left a comment

Choose a reason for hiding this comment

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

Just one recurring comment about json tags but the rest look good.
I think you may be missing an update to the deepcopy function (I had one in my PR but forgot what generated it 🤔 )

@roncodingenthusiast roncodingenthusiast force-pushed the NET-5346 branch 2 times, most recently from d4c918b to 027730c Compare September 12, 2023 18:32
wilkermichael
wilkermichael approved these changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@wilkermichael wilkermichael left a comment

Choose a reason for hiding this comment

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

Great work here! Some really minor comments

Thanks for testing this locally as well :D

control-plane/api/v1alpha1/jwtprovider_types.go Outdated Show resolved Hide resolved
control-plane/api/v1alpha1/jwtprovider_types.go Show resolved Hide resolved
@roncodingenthusiast roncodingenthusiast merged commit d2fe5ed into main Sep 14, 2023
3 checks passed
@roncodingenthusiast roncodingenthusiast deleted the NET-5346 branch September 14, 2023 14:45
@hc-github-team-consul-core hc-github-team-consul-core mentioned this pull request Sep 14, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilkermichael wilkermichael wilkermichael approved these changes

@kisunji kisunji kisunji approved these changes

@skpratt skpratt Awaiting requested review from skpratt

Assignees
No one assigned
Labels
backport/1.2.x This release branch is no longer active.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JWKSCluster missing from CRD handling
3 participants
@roncodingenthusiast @kisunji @wilkermichael