Tracking issue for pending feedbacks/Issues in PR83(Notation CLI baseline)

[priteshbandi]

Tracking issue for multiple comments/issues that were called out in #83 and needs to be resolved.

Issues marked with [Alpha], must to be resolved for alpha release.(We can discuss about prioritization in next meeting)

Security

• [Alpha] Don’t take password from CLI in plain text; Also taking username and password for each command doesn’t looks like a good UX. (PB: Same as that of [Feature] notation login  #119)

Readability & Maintainability

• [Alpha] Remove all dead/unused code.
• [Alpha] For better readability, at minimum we should to add documentation for important packages and exported names. (PB: Being taken care part of refactoring notation-go and notaiton-core-go)
• [Alpha] The package structure should be reorganized for better readability.
• [Alpha] Add unit tests. At minimum we need test which exercises positive scenario(successful signing and verification) to avoid breaking changes. (PB: Still valid)
• Update config.go to export functions instead of exposing internal implementation so that we can modify internals without affecting callers. (PB: Still valid)
• Move default local file-based signer and verifier implementation to notatio-go-lib

Usability

• Need better error messages to display in case of failure (PB: Still valid) --> Covered in Improve error messages for notation CLI #128
• The CLIs should exit with proper exit code(non zero for failures) Ref (PB: Still valid)
• [Alpha] Verifier CLI should just take list of certs that user trust for verification. No need of separate ca cert param.
• Don't overwrite files if file exists unless explicitly indicated by user

Bugs

• User shouldn't be able to add two cert/keys with same name

Nit-picks

• Move SignatureDigests from path.go to cache.go
• Rename packages to be more relevant: E.g.: cache.go, list.go (PB: Not required)
• Whenever shellingout read both error and std streams (PB: Still valid). Replaced with Whenever shelling out read both error and std streams for Notation client #280
• add 127.0.0.1 for checking localhost (PB: Still valid) Replaced with Remove hardcoded reference to "localhost" #279
• reuse common code between docker-notation/docker/manifest.go and docker-generate/manifest. (PB: Still valid)

[dtzar]

@priteshbandi - taking a quick look, it seems a number of these are resolved. Can you please update your checklist?
Remaining items which don't have an issue we can create and put into RC-1.

[dtzar]

I migrated the top two issues. @iamsamirzon once you migrate the bottom three open ones above, we can close this out.