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

[WIP] Update testcase for cert plugin #503

Closed
wants to merge 1 commit into from

Conversation

Akasurde
Copy link
Member

Fixes https://fedorahosted.org/freeipa/ticket/6275

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
Copy link
Contributor

@MartinBasti MartinBasti left a comment

Choose a reason for hiding this comment

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

I still miss tests for cert-find and following attributes:

  • Issuing CA
  • Subject
  • Issuer
  • Serial number
  • Serial number (hex)
  • Status
  • Revoked

Is possible to implement a Tracker for certificates and create a testing certificate and compare all expected and received values?

@@ -191,10 +192,10 @@ def test_0005_cert_uris(self):

See https://fedorahosted.org/freeipa/ticket/5881
"""
result = api.Command.cert_show(sn, out=unicode(self.certfile))
api.Command['cert_show'](sn, out=unicode(self.certfile))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

result is unused and wanted to make api call uniform with other calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Show status of certificate using request id and CA
"""
res = api.Command['cert_status'](request_id=1,
cacn=unicode('ipa'))['result']
Copy link
Contributor

Choose a reason for hiding this comment

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

please use u'ipa' instead of unicode(ipa)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

"""
Show status of certificate using request id
"""
res = api.Command['cert_status'](request_id=1)['result']
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, can we always assume that request with id=1 is there?

Copy link
Member Author

Choose a reason for hiding this comment

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

We will surely have request_id=1 which is used for IPA CA certificate. That's why I used request id 1. We can change this if this is not the case.

fp = open(self.pwname, "w")
fp.write("\n")
fp.close()
with open(self.pwname, "w") as fp:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in separate commit

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

@@ -122,9 +124,8 @@ def generateCSR(self, subject):
"-f", self.pwname,
"-a",
])
fp = open(self.reqfile, "r")
data = fp.read()
fp.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

This refactoring should be in separate commit

@@ -50,6 +50,7 @@
_EXP_CRL_URI = ''.join(['http://ipa-ca.', _DOMAIN, '/ipa/crl/MasterCRL.bin'])
_EXP_OCSP_URI = ''.join(['http://ipa-ca.', _DOMAIN, '/ca/ocsp'])


Copy link
Contributor

Choose a reason for hiding this comment

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

This PEP8fix should be in separate commit

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I will keep this in mind for future.

@@ -97,15 +100,14 @@ def run_certutil(self, args, stdin=None):
return ipautil.run(new_args, stdin)

def setup(self):
self.reqdir = tempfile.mkdtemp(prefix = "tmp-")
self.reqdir = tempfile.mkdtemp(prefix="tmp-")
Copy link
Contributor

Choose a reason for hiding this comment

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

Pep8 fix should be in separate commit

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

@MartinBasti
Copy link
Contributor

I left some inline comments, this improves the test but it still misses several features to be tested.
You can finish these improvements and it can be pushed and add more improvements in a new PR

@MartinBasti MartinBasti self-assigned this Feb 24, 2017
@Akasurde
Copy link
Member Author

Akasurde commented Feb 24, 2017

@MartinBasti I am working on other improvements and will update this PR accordingly.

  • Issuing CA
  • Subject
  • Issuer
  • Serial number
  • Serial number (hex)
  • Status
  • Revoked

@MartinBasti
Copy link
Contributor

@Akasurde what is your opinion about creating a Tracker class for certificate?

@Akasurde
Copy link
Member Author

@MartinBasti Will implement tracker class in different PR.

@MartinBasti MartinBasti removed their assignment May 26, 2017
@tkrizek
Copy link
Contributor

tkrizek commented Aug 29, 2017

Closing for inactivity. If this PR is still relevant, please re-open it.

@tkrizek tkrizek closed this Aug 29, 2017
@tkrizek tkrizek added the rejected Pull Request has been rejected label Aug 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rejected Pull Request has been rejected
Projects
None yet
3 participants