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

Simplify credential normalization, fix tests #1318

Merged
merged 2 commits into from
Jul 28, 2021

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jul 27, 2021

  • Don't build a map just to access one element. NOTE: This might change behavior with ambiguous files, we now use the first matching entry, not the last one.
  • Don't unnecessarily strip schema:// and /path from registry on search, and adjust tests

See the individual commit messages for details.

mtrmac added 2 commits July 27, 2021 19:53
Instead, just iterate and look for the key we want.

NOTE: This might change behavior with ambiguous files,
we now use the first matching entry, not the last one.

Signed-off-by: Miloslav Trmač <[email protected]>
... because the value, at that point. is really supposed to be
only a registry.

Also adjust failing tests; they were added by commit
d30079f . At that point,
the argument to getAuth() only could have been a registry,
so the test cases were never representative; typically
they tested normalization of the lookup key instead of the
config entry, when the config entry was what needed
normalizing.  So, move the URLs and paths to the config file,
and remove them from the lookup query, when relevant.

Also rework TestGetAuthFromLegacyFile a bit more significantly,
so that the test descriptions and test contents actually match
instead of being exactly opposite.

Signed-off-by: Miloslav Trmač <[email protected]>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@vrothberg vrothberg merged commit a4c7ee7 into containers:main Jul 28, 2021
@mtrmac mtrmac deleted the normalize-tests branch July 28, 2021 12:39
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