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 OIDC role mapping #137

Merged
merged 5 commits into from
Oct 10, 2024
Merged

Conversation

akhter-ali-idexx
Copy link
Contributor

No description provided.

@alexhung alexhung added the enhancement Automatically generated release notes label Oct 9, 2024
Copy link
Member

@alexhung alexhung left a comment

Choose a reason for hiding this comment

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

@@ -97,11 +97,11 @@ func (r *odicIdentityMappingResource) Schema(ctx context.Context, req resource.S
Required: true,
Validators: []validator.String{
stringvalidator.RegexMatches(
regexp.MustCompile(`^(applied-permissions\/admin|applied-permissions\/user|applied-permissions\/groups:.+)$`),
"must start with either 'applied-permissions/admin', 'applied-permissions/user', or 'applied-permissions/groups:'",
regexp.MustCompile(`^(applied-permissions\/admin|applied-permissions\/user|applied-permissions\/groups|applied-permissions\/roles:.+)$`),
Copy link
Member

Choose a reason for hiding this comment

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

The trailing : is missing for the groups.

Copy link
Member

Choose a reason for hiding this comment

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

The : is still missing. Should be applied-permissions\/groups:

Copy link

github-actions bot commented Oct 9, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@akhter-ali-idexx
Copy link
Contributor Author

Please also add test for the roles scope. See https://github.com/jfrog/terraform-provider-platform/blob/main/pkg/platform/resource_oidc_identity_mapping_test.go#L130 for example.

Done

@alexhung
Copy link
Member

alexhung commented Oct 9, 2024

Please also add test for the roles scope. See https://github.com/jfrog/terraform-provider-platform/blob/main/pkg/platform/resource_oidc_identity_mapping_test.go#L130 for example.

Done

@akhter-ali-idexx Excellent!

Can you also try to sign the CLA? The bot may fails but that's ok as long as you sign.

@akhter-ali-idexx
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@alexhung
Copy link
Member

alexhung commented Oct 9, 2024

@akhter-ali-idexx See test failures
Screenshot 2024-10-09 at 9 52 08 AM

@akhter-ali-idexx
Copy link
Contributor Author

@akhter-ali-idexx See test failures Screenshot 2024-10-09 at 9 52 08 AM

Wow, I completely missed that, lol. Test execution added

@akhter-ali-idexx
Copy link
Contributor Author

@alexhung Please do let me know if I've missed anything else, I'd like to use asap. Thank you!

Copy link
Member

@alexhung alexhung left a comment

Choose a reason for hiding this comment

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

Look good enough for now. I'll take this from here and get it to be in a releasable state.

@@ -97,11 +97,11 @@ func (r *odicIdentityMappingResource) Schema(ctx context.Context, req resource.S
Required: true,
Validators: []validator.String{
stringvalidator.RegexMatches(
regexp.MustCompile(`^(applied-permissions\/admin|applied-permissions\/user|applied-permissions\/groups:.+)$`),
"must start with either 'applied-permissions/admin', 'applied-permissions/user', or 'applied-permissions/groups:'",
regexp.MustCompile(`^(applied-permissions\/admin|applied-permissions\/user|applied-permissions\/groups|applied-permissions\/roles:.+)$`),
Copy link
Member

Choose a reason for hiding this comment

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

The : is still missing. Should be applied-permissions\/groups:

@alexhung alexhung merged commit 3cd723d into jfrog:main Oct 10, 2024
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Automatically generated release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants