Skip to content

ISD-2183 Add certificates integration tests#7

Merged
Thanhphan1147 merged 94 commits intomainfrom
add_certificates_relation_tests
Oct 15, 2024
Merged

ISD-2183 Add certificates integration tests#7
Thanhphan1147 merged 94 commits intomainfrom
add_certificates_relation_tests

Conversation

@Thanhphan1147
Copy link
Collaborator

@Thanhphan1147 Thanhphan1147 commented Aug 13, 2024

Applicable spec: ISD163 (internal)

Add unit and integration tests for the new tls_relation module

Checklist

@Thanhphan1147 Thanhphan1147 changed the title Add certificates integration tests ISD-2183 Add certificates integration tests Sep 30, 2024
@github-actions
Copy link
Contributor

Test coverage for 85b3c71

Name                      Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------
src/charm.py                 83     41     12      0    44%   71-72, 77-78, 83, 92-94, 103-106, 117-124, 131-132, 140-153, 157-159, 163-178
src/haproxy.py               47     10      2      1    78%   63, 71-72, 88-91, 101-103
src/state/config.py          40     22      6      0    39%   50-70, 85-93, 105-106
src/state/tls.py             38     12      6      1    61%   55-68, 91-96
src/state/validation.py      31     17      6      0    38%   59-75
src/tls_relation.py         117      2     18      1    96%   254-255, 263->266
---------------------------------------------------------------------
TOTAL                       356    104     50      3    66%

Static code analysis report

Run started:2024-10-14 10:11:42.665208

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1238
  Total lines skipped (#nosec): 0
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 3

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

Copy link
Contributor

@amandahla amandahla 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
Contributor

@alithethird alithethird left a comment

Choose a reason for hiding this comment

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

Left 2 comments also you have a typo in src/haproxy.py s/haporxy/haproxy.
I have a couple more notes:
1- I think you can put the TLSInformation.validate() in a decorator. I think you can even put it in the validate_config_and_tls decorator and activate with a parameter like validate_info. You are calling it in 4 different functions.
2- Why use typing.cast(str, ...) instead of str(...)? As far as I know typing.cast() doesn't actually cast it, it just silences the mypy etc. doc

@Thanhphan1147
Copy link
Collaborator Author

Left 2 comments also you have a typo in src/haproxy.py s/haporxy/haproxy. I have a couple more notes: 1- I think you can put the TLSInformation.validate() in a decorator. I think you can even put it in the validate_config_and_tls decorator and activate with a parameter like validate_info. You are calling it in 4 different functions. 2- Why use typing.cast(str, ...) instead of str(...)? As far as I know typing.cast() doesn't actually cast it, it just silences the mypy etc. doc

I didn't put TLSInformation.validate() in a decorator because the hook was following the general flow of initializing the chamstate component for the hook -> do business logic and any errors will be caught and handled by the decorator. In cases where the return value of the charmstate component is not needed, then we'll use the validate() method of the charmstate component class. You can imagine different hooks can require any combination of charmstate components so putting all of them in a decorator and add a bunch of parameters is a bit messy. Also it doesn't make much sense if tls_information = TLSInformation.from_charm(self, self.certificates) and TLSInformation.validate(self) are in different places when they are functionally the same.

For typing.cast the main purpose for it is actually to tell mypy that at that point either the cert or the provider cert must exists

Copy link
Contributor

@alithethird alithethird left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Thanhphan1147 Thanhphan1147 merged commit ba5383a into main Oct 15, 2024
@Thanhphan1147 Thanhphan1147 deleted the add_certificates_relation_tests branch October 15, 2024 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

complex documentation Improvements or additions to documentation Libraries: Out of sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants