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

Update Packaging #65

Merged
merged 9 commits into from
Oct 26, 2023
Merged

Update Packaging #65

merged 9 commits into from
Oct 26, 2023

Conversation

cherbel
Copy link
Member

@cherbel cherbel commented Oct 24, 2023 •
edited
Loading

  • Updates to Javascript SDK for packaging (Now published on NPM)
  • Removes old app directory (no longer used)
  • Refactors Server
    • Refactors server to use new rebuff NPM package
    • General cleanup and consolidation of all dependencies into the server directory
    • Updates server install steps in readme
  • Removes mono-repo package files and artifacts that are no longer needed.

@cherbel cherbel added the github-releases Related to package release label Oct 24, 2023
@cherbel cherbel requested a review from seanpmorgan October 24, 2023 17:39
@cherbel cherbel self-assigned this Oct 24, 2023
@cherbel cherbel added documentation Improvements or additions to documentation and removed github-releases Related to package release labels Oct 24, 2023
@cherbel cherbel linked an issue Oct 24, 2023 that may be closed by this pull request
Publish updated rebuff npm package #53
Closed
@seanpmorgan seanpmorgan marked this pull request as ready for review October 24, 2023 21:55
@seanpmorgan seanpmorgan marked this pull request as draft October 24, 2023 21:58
@cherbel cherbel changed the title Update Packaging Documentation Update Packaging Oct 24, 2023
@cherbel cherbel marked this pull request as ready for review October 25, 2023 00:16
seanpmorgan
seanpmorgan requested changes Oct 25, 2023
View reviewed changes
Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Almost LGTM, small issue with our admittedly convoluted test setup (will be improved near term)

Makefile
Comment on lines -16 to -19

init: init-python-sdk init-server
npm install

Copy link
Member

Choose a reason for hiding this comment

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

So this is called in our github workflow:
https://github.com/protectai/rebuff/blob/main/.github/workflows/python_tests.yaml#L27-L28
https://github.com/protectai/rebuff/blob/main/python-sdk/tests/conftest.py#L21-L58

Which stands up the server for python-sdk tests (and subsequently tests ingration with the server -- now pinned to a specific release). Ultimately we should replace this setup with unit testing individual TS functions but for the time being is there a need to remove this?

Copy link
Member Author

@cherbel cherbel Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Added it back, although I removed the npm install, because the init-server step runs the install in the server directory, and there is no longer a package.json in the root directory.

README.md Outdated Show resolved Hide resolved
@cherbel cherbel requested a review from seanpmorgan October 25, 2023 23:48
seanpmorgan
seanpmorgan approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@seanpmorgan seanpmorgan 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!

@seanpmorgan seanpmorgan merged commit 90630cb into main Oct 26, 2023
1 check passed
@seanpmorgan seanpmorgan deleted the cherbel-javascript-packaging branch October 26, 2023 22:44
@seanpmorgan seanpmorgan mentioned this pull request Oct 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanpmorgan seanpmorgan seanpmorgan approved these changes

Assignees

@cherbel cherbel

Labels
documentation Improvements or additions to documentation okay-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Publish updated rebuff npm package
2 participants
@cherbel @seanpmorgan