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.

Sign up for GitHub

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

Jump to bottom

feat: refactor certs generation and add tests #104

Merged
merged 19 commits into from
May 30, 2023
Merged

feat: refactor certs generation and add tests #104

merged 19 commits into from
May 30, 2023

Conversation

NohaIhab
Copy link
Contributor

Summary:

  • use clean implementation for generating certs
  • add unit tests to check on certs generation and its contents

@NohaIhab NohaIhab requested a review from a team as a code owner April 27, 2023 13:38
@NohaIhab
feat: more refactoring of cert generation
2004839 
- moved gen_certs to certs.py
- added gen_certs_if_missing
- test gen_certs_if_missing
- remove unneeded mocked_gen_certs fixture
@NohaIhab
fix: add certs.py
8b872d8
beliaev-maksim
beliaev-maksim previously requested changes May 8, 2023
View reviewed changes
charms/kserve-controller/src/charm.py Outdated Show resolved Hide resolved
@ca-scribner
Copy link
Contributor

Is this PR still in work? I see some test failures but I don't think this landed elsewhere

@NohaIhab
Copy link
Contributor Author

NohaIhab commented May 17, 2023 •
edited
Loading

Is this PR still in work? I see some test failures but I don't think this landed elsewhere

@ca-scribner
yes it's a WIP, it's in my queue this pulse to finalize it.

@NohaIhab
Copy link
Contributor Author

do we want to close this in favor of using tls operator?

@i-chvets
Copy link
Contributor

do we want to close this in favor of using tls operator?

We need to finish it, so kserve can function. What's left?

@NohaIhab
Copy link
Contributor Author

We need to finish it, so kserve can function. What's left?

  • It's functioning alright, but Andrew suggested to use a library rather than openssl calls, but I'm having issues with the pyopenssl lib. we can take this offline.
  • not sure why istio is not reaching active in integration tests.

DnPlas
DnPlas reviewed May 22, 2023
View reviewed changes
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @NohaIhab, few comments.

charms/kserve-controller/src/certs.py Outdated Show resolved Hide resolved
charms/kserve-controller/src/certs.py Outdated Show resolved Hide resolved
charms/kserve-controller/src/certs.py Outdated Show resolved Hide resolved
charms/kserve-controller/src/charm.py Outdated Show resolved Hide resolved
charms/kserve-controller/tests/unit/test_charm.py Show resolved Hide resolved
charms/kserve-controller/src/charm.py Outdated Show resolved Hide resolved
charms/kserve-controller/src/charm.py Outdated Show resolved Hide resolved
charms/kserve-controller/src/charm.py Outdated Show resolved Hide resolved
charms/kserve-controller/src/charm.py Outdated Show resolved Hide resolved
charms/kserve-controller/src/charm.py Outdated Show resolved Hide resolved
DnPlas
DnPlas approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @NohaIhab

@NohaIhab NohaIhab dismissed beliaev-maksim’s stale review May 30, 2023 10:01

change request was addressed

@NohaIhab NohaIhab merged commit ba760a9 into main May 30, 2023
@NohaIhab NohaIhab deleted the KF-1730-gen-certs branch May 30, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ca-scribner ca-scribner ca-scribner left review comments

@beliaev-maksim beliaev-maksim beliaev-maksim left review comments

@i-chvets i-chvets i-chvets left review comments

@DnPlas DnPlas DnPlas approved these changes

Assignees
No one assigned
Labels
Libraries: OK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NohaIhab @ca-scribner @i-chvets @DnPlas @beliaev-maksim