Update validateUILayouts logic #21
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
denisonbarbosa
commented
Aug 17, 2023
denisonbarbosa
force-pushed
the
valid-layouts-update
branch
from
0f771f3
to
f36e6ae
Compare
denisonbarbosa
commented
Aug 18, 2023
GabrielNagy
reviewed
Aug 21, 2023
GabrielNagy
approved these changes
Aug 21, 2023
didrocks
requested changes
Aug 28, 2023
There was a problem hiding this comment.
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.
denisonbarbosa
force-pushed
the
valid-layouts-update
branch
from
c4d3e68
to
8b896ea
Compare
|
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? |
denisonbarbosa
force-pushed
the
valid-layouts-update
branch
from
4423285
to
97cdc33
Compare
didrocks
requested changes
Aug 29, 2023
.../testdata/TestGetAuthenticationModes/golden/get_authentication_modes_and_generate_validators
Show resolved
Hide resolved
didrocks
approved these changes
Aug 29, 2023
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
force-pushed
the
valid-layouts-update
branch
from
f8cbb0f
to
28f75c9
Compare
denisonbarbosa
force-pushed
the
valid-layouts-update
branch
from
28f75c9
to
5c4a1dc
Compare
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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