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

feat: Seldon rock integration #12

Merged
merged 17 commits into from
Jun 15, 2023
Merged

Conversation

i-chvets
Copy link
Contributor

@i-chvets i-chvets commented Jun 2, 2023

NOTE: This PR description only contains information related to code changes and testing, if required. All detailed information about problem statement, design, implementation, technical discussions, etc. is tracked in corresponding Github issue indicated below. This will ensure that important information is reliably recorded and tracked and not scattered across PR(s).

Details
Refer to the following Github issue for more information on feature/fix that this PR is related to:
canonical/seldon-core-operator#133

Summary of changes:
- Updated ROCK definition.
- Updated tests.
- Changed service in ROCK to match service that is launched via Charm.
- Use bash shell and yq instead of CheckRock test class due to size of charmed-kubeflow-chisme package of 200KB.

NOTE: Use of bash shell commands significantly reduces maintability of tox.ini ACK: @kimwnasptd

Ivan Chvets added 13 commits May 25, 2023 20:26
Summary of changes:
- Added tox.ini to test rock.
- Added unit tests to test rock's integrity (passing)
- Added integration tests (not passing, needs update from Seldon Core
  Operator integration tests)
Summary of changes:
- Modified tox.ini according to code review comments.
- Updated security requirements part.
- Moved rock to use bare base.
Summary of changes:
- Reorganized tox.ini
- Updated tests to use filesystem.
Summary of changes:
- Added fixture to test.
Summary of changes:
- Added fixture to test.
…onical/seldonio-rocks into kf-3006-feat-seldon-rock-integration
Summary of changes:
- Re-organise tox to avoid not needed microk8s setup.
Summary of changes:
- Updated tox.ini to run tests.
Summary of changes:
- Updated ROCK definition.
- Updated tests.
@i-chvets i-chvets requested a review from a team as a code owner June 2, 2023 20:41
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 @i-chvets, some comments.

seldon-core-operator/rockcraft.yaml Show resolved Hide resolved
seldon-core-operator/tox.ini Outdated Show resolved Hide resolved
seldon-core-operator/rockcraft.yaml Outdated Show resolved Hide resolved
@i-chvets
Copy link
Contributor Author

i-chvets commented Jun 6, 2023

CheckRock test class: canonical/charmed-kubeflow-chisme#56

Summary of changes:
- Added test to verify that service name in ROCK matches the service
  name in the corresponding charm.
@i-chvets
Copy link
Contributor Author

@DnPlas @kimwnasptd Let's resolve/approve this issue.

@i-chvets i-chvets changed the title Kf 3006 feat seldon rock integration feat: Seldon rock integration Jun 14, 2023
Ivan Chvets added 2 commits June 14, 2023 19:38
canonical/seldon-core-operator#133

Summary of changes:
- Updated rockcraft.yaml with new run-user option to run as non-root.
- Updated import procedure.
Summary of changes:
- Reverted to use of shell commands and `yq` instead of CheckRock test
  class from chisme package.

NOTE: Use of bash shell commands significantly reduces maintability of
tox.ini
@i-chvets
Copy link
Contributor Author

@DnPlas @kimwnasptd Reverted to use of bash shell sripting and yq instead of CheckRock test class. Ready for approval.

ca-scribner
ca-scribner previously approved these changes Jun 15, 2023
@i-chvets i-chvets merged commit c71f4cb into main Jun 15, 2023
@i-chvets i-chvets deleted the kf-3006-feat-seldon-rock-integration branch June 15, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants