Skip to content

fix: allow empty credentials store config#286

Merged
gokarnm merged 1 commit intonotaryproject:mainfrom
JeyJeyGao:fix/allow_empty_credentials
Aug 9, 2022
Merged

fix: allow empty credentials store config#286
gokarnm merged 1 commit intonotaryproject:mainfrom
JeyJeyGao:fix/allow_empty_credentials

Conversation

@JeyJeyGao
Copy link
Copy Markdown
Contributor

@JeyJeyGao JeyJeyGao commented Aug 9, 2022

Backgroud: according to previous design, credentials store config must exist in notation config.json or docker config.json, but you may not need a credentials store config to sign image for local registry.

Fix: we need to allow empty credentials store config for local reigistry.

Test:

  1. sign local registry
  2. sign remote registry with username/password
  3. sign remote registry without username/password
  4. login with credential config
  5. login without credential config
  6. logout with credential config
  7. logout without credential config

Resolves #284
Signed-off-by: Junjie Gao junjiegao@microsoft.com

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 9, 2022

Codecov Report

Merging #286 (d7e3b2d) into main (1356216) will decrease coverage by 0.66%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
- Coverage   61.73%   61.07%   -0.67%     
==========================================
  Files           9        9              
  Lines         277      280       +3     
==========================================
  Hits          171      171              
- Misses        100      102       +2     
- Partials        6        7       +1     
Impacted Files Coverage Δ
pkg/auth/credential.go 92.50% <25.00%> (-7.50%) ⬇️
pkg/auth/native_store.go 90.69% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
@JeyJeyGao JeyJeyGao force-pushed the fix/allow_empty_credentials branch from 62fa8a9 to d7e3b2d Compare August 9, 2022 07:06
Copy link
Copy Markdown
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@binbin-li binbin-li left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Contributor

@gokarnm gokarnm left a comment

Choose a reason for hiding this comment

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

LGTM!

@gokarnm gokarnm merged commit b5b56e0 into notaryproject:main Aug 9, 2022
@dtzar
Copy link
Copy Markdown

dtzar commented Aug 9, 2022

Thank you @FeynmanZhou for finding this, @JeyJeyGao for the quick fix turnaround, and quick review from @gokarnm @binbin-li @shizhMSFT 👍

@JeyJeyGao JeyJeyGao deleted the fix/allow_empty_credentials branch December 1, 2022 05:58
7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
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.

Credentials store config is not set up when using notation sign

6 participants