don't raise error in get if certificate is empty #84
Conversation
rishichawda
force-pushed
the
shouldnot-raise-error-for-empty-certificate-in-get-call
branch
from
ed5ec4b to
6a721c9
Compare
rishichawda
changed the title
rishichawda
force-pushed
the
shouldnot-raise-error-for-empty-certificate-in-get-call
branch
3 times, most recently
from
0008172 to
80cbd7b
Compare
Signed-off-by: rishichawda <[email protected]>
Signed-off-by: rishichawda <[email protected]>
rishichawda
force-pushed
the
shouldnot-raise-error-for-empty-certificate-in-get-call
branch
from
80cbd7b to
d94d5b6
Compare
Signed-off-by: rishichawda <[email protected]>
rishichawda
force-pushed
the
shouldnot-raise-error-for-empty-certificate-in-get-call
branch
from
d94d5b6 to
c52c0fd
Compare
rishichawda
force-pushed
the
shouldnot-raise-error-for-empty-certificate-in-get-call
branch
2 times, most recently
from
f5cb4e1 to
48363eb
Compare
Signed-off-by: rishichawda <[email protected]>
rishichawda
force-pushed
the
shouldnot-raise-error-for-empty-certificate-in-get-call
branch
from
48363eb to
0049fca
Compare
|
Rishi, I think I found a couple of the problems. I am also working out some issues with the unit tests failing. |
|
@johnmccrae Please let me know if there's something I can help with. |
| def valid?(certificate_thumbprint, store_location: "", store_name: "") | ||
| cert_validate(certificate_thumbprint, store_location: store_location, store_name: store_name) | ||
| cert_validate(certificate_thumbprint, store_location: store_location, store_name: store_name).yield_self do |x| | ||
| if x.is_a?(TrueClass) || x.is_a?(FalseClass) | ||
| x | ||
| else | ||
| false | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
I think the implementations of valid? and cert_validate should have been other way round or somewhat swapped, i.e. valid? should contain the validation logic that cert_validate has, returning a bool, while cert_validate should use valid? to check and raise error or formatted error string. Something like below -
def valid?
# validation logic
# return boolean
end
def cert_validate
# use valid? to check
# raise error or return formatted error string (based on use case)
end| def get!(certificate_thumbprint, store_name: @store_name, store_location: @store_location) | ||
| cert_pem = cert_get(certificate_thumbprint, store_name: store_name, store_location: store_location) | ||
|
|
||
| raise ArgumentError, "Unable to retrieve the certificate" if cert_pem.empty? |
There was a problem hiding this comment.
ArgumentError would be fitting if we were validating arguments here but since we are calling an external entity/store, we should either relay/wrap that entity's error or define a custom exception.
| unless cert_pem.empty? | ||
| build_openssl_obj(cert_pem) | ||
| end | ||
| cert_pem.empty? ? cert_pem : build_openssl_obj(cert_pem) |
There was a problem hiding this comment.
I think this should return nil or cert_pem/object. This would also clean .empty? checks at other places.
There was a problem hiding this comment.
Can we get on Teams and talk about this? I feel like we have competing agendas and I want to make sure I am doing this correctly.
Signed-off-by: rishichawda [email protected]
Description
Currently
cert_getcall is checking if the value returned byget_cert_pemforpowershell_exec!output is empty, and raises an error. However, we do have acert_validatethat achieves the same thing by callingverify_certificatewhich will return "Certificate not found" as string. It doesn't throw any errors, and only returns the string with the error message. This should also be eliminated since thevalid?call is expected to return eithertrueorfalse.Complete details on the idea behind these changes are in #83
Issues Resolved
#83
chef/chef#11994
[List any existing issues this PR resolves, or any Discourse or
StackOverflow discussions that are relevant]
Check List