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

Update validateUILayouts logic #21

Merged
merged 5 commits into from
Aug 29, 2023
Merged

Update validateUILayouts logic #21

merged 5 commits into from
Aug 29, 2023

Conversation

denisonbarbosa
Copy link
Member

We used to have a static verification for the UI layouts returned by the broker, but those are subject to change according to what the system can currently handle. This means that it's up to PAM to provide which layouts are supported, which fields of those layouts are required and what values those fields support.

This replaces the previous static implementation of validateUILayouts with a dynamic one that takes into consideration what was specified by PAM.

UDENG-1166

@denisonbarbosa denisonbarbosa requested a review from a team as a code owner August 17, 2023 15:29
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

I really like this one, it’s clean (I have some suggestions/requests for more thoughts on the naming), but the validation function is really what I was thinking about.

I think the testing though needs to be digged in: having more use cases and, in particular, more precise one, not linking with the layout we support, because apart from "type" (which is one of the case to tests, the rest is free form, and we can reflect what we are testing in the input name.

internal/brokers/broker_test.go Outdated Show resolved Hide resolved
internal/brokers/broker_test.go Outdated Show resolved Hide resolved
internal/brokers/broker.go Outdated Show resolved Hide resolved
internal/brokers/broker_test.go Outdated Show resolved Hide resolved
internal/brokers/broker.go Show resolved Hide resolved
internal/brokers/broker.go Show resolved Hide resolved
internal/brokers/broker.go Outdated Show resolved Hide resolved
@didrocks
Copy link
Member

Ok, so I think you missed 2 comments that are addressable right now, and if I get you right, you prefer to have pr 22 landed before doing the test changes (even if I don’t forsee any conflicts TBH, looking at both), correct?

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

excellent! As said, I love the fixes and in particular the new mock test approach which is independent of the layout type. I have some minor comments that should be easy to address!

internal/brokers/broker.go Outdated Show resolved Hide resolved
internal/testutils/broker.go Show resolved Hide resolved
internal/brokers/broker_test.go Outdated Show resolved Hide resolved
internal/brokers/broker_test.go Outdated Show resolved Hide resolved
internal/brokers/export_test.go Show resolved Hide resolved
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

One the test case being clarified, feel free to stash and merge! This is way better :)

internal/brokers/broker_test.go Outdated Show resolved Hide resolved
We used to have a static verification for the UI layouts returned by the
broker, but those are subject to change according to what the system can
currently handle.

This means that it's up to PAM to provide which layouts are supported,
which fields of those layouts are required and what values those fields
support.

This replaces the previous static implementation of validateUILayouts to
a dinamic one that takes into consideration what was specified by PAM.
Since this a private function only called in GetAuthenticationModes, we
need to export it to avoid having to call GAM in the
SelectAuthenticationMode tests.

We also added a helper to translate the validators into a JSON-like
string to help with the tests.
@denisonbarbosa denisonbarbosa merged commit 4613420 into main Aug 29, 2023
3 checks passed
@denisonbarbosa denisonbarbosa deleted the valid-layouts-update branch August 29, 2023 14:31
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