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

Feature/add Syftbox install comand #25

Merged
merged 22 commits into from
Oct 11, 2024
Merged

Feature/add Syftbox install comand #25

merged 22 commits into from
Oct 11, 2024

Conversation

IonesioJunior
Copy link
Member

@IonesioJunior IonesioJunior commented Oct 3, 2024 •
edited
Loading

Creates a syftbox app install command.

Acceptance Criteria:

Features:

  • Must install and run a new app with its settings.
  • Handle gracefully if git isn't installed.
  • Handle gracefully if repository doesn't exist.
  • Handle gracefully if config.json doesn't exist.
  • Handle gracefully if config.json version isn't compatible.
  • Handle gracefully if pre-install returns an error.
  • Handle gracefully if post install returns an error.
  • Handle gracefully if run returns an error.
  • Update ~/.syftbox/apps.json.

Tests:

  • Test Handling gracefully if git isn't installed.
  • Test Handling gracefully if config.json version isn't compatible.
  • Test Handling gracefully if repository doesn't exist.
  • Test Handling gracefully if config.json doesn't exist.
  • Test Handling gracefully if pre-install returns an error.
  • Test Handling gracefully if post install returns an error.
  • Test Handling gracefully if run returns an error.

CODE QUALITY GUIDELINES:

  • NO unecessary print statements.
  • Documented code.

@IonesioJunior
feat(syftbox/app_manager): ADD new syftbox/app_manager submodule
e0e91fa 
Adds a submodule to perform syftbox install <app repository>
@IonesioJunior
feat(syftbox/main.py): ADDS install as a valid arg for syftbox CLI
ca7680f 
syftbox CLI will use app_manager to install app repositories
@iamtrask
Copy link
Member

iamtrask commented Oct 3, 2024

watched the video - compelling idea

IonesioJunior and others added 20 commits October 8, 2024 10:17
@IonesioJunior
Merge remote-tracking branch 'origin/main' into feature/add_syftbox_i…
87fee0f 
…nstall
@IonesioJunior
Merge remote-tracking branch 'origin/main' into feature/add_syftbox_i…
f3358bc 
…nstall
@IonesioJunior
feat(syftbox/main.py): ADD new app command to syftbox cli
c5bd408 
app command will be responsible for installing and managing new syftbox apps
@IonesioJunior
refactor(syftbox/app_manager): RENAME app_manager submodule to just app
10d7fad 
Rename app manager submodule to app
@IonesioJunior
feat(syftbox/install.py): ADD a new source file responsible for app i…
32bf893 
…nstallation workflow
@IonesioJunior
feat(syftbox/app/utils.py): ADD small syftbox/app/utils.py source file
e839d46 
Add a utils.py source file to share common variables/functions in the module
@IonesioJunior
Merge branch 'main' into feature/add_syftbox_install
ac2c6f5
@IonesioJunior
fix(syftbox/app/manager.py): FIX SYFT_CLIENT_CONFIG path exception
d4039ba 
Handle when SYFT_CLIENT_CONFIG_PATH exception when the env var isn't set
@IonesioJunior
refactor(syftbox/app/manager.py-syftbox/app/utils.py): REMOVE unused …
62a2d7a 
…constant, move shared function to utils.py

The base_path constant isn't required anymore, instead we must use the get_config_path
@IonesioJunior
feat(syftbox/apps/install.py): ADD the update_app_json at the end of …
d185fc8 
…app installation workflow

This app.json will be responsible to keep track of which apps are installed and up to date
@IonesioJunior
feat(syftbox/client/plugins/app.py): Empower each app to have their o…
2e235e5 
…wn interval schedule defined by their app.json config file

Most of apps will have different requirements in terms of refresh rate. To achieve that, we must empower the app config to tell the system how often they must run.
@IonesioJunior
Merge branch 'main' into feature/add_syftbox_install
a1b562b
@IonesioJunior
fix(syftbox/app/install.py): Fix small uncovered workflows and handle…
4816283 
… them properly

Cover use cases where the given repository path doesn't exist / app.json is invalid or isn't found
@IonesioJunior
test(tests/client/app_manager/install_test.py): ADD a set of tests to…
240e132 
… cover syftbox app install workflow
@IonesioJunior
style(syftbox/app/install.py): REMOVE unused/uncessary print statements
9c4edb3
@IonesioJunior
fix(syftbox/client/plugins/app.py): Uncomment the copy_default_apps f…
7793ae8 
…unction

During development process i turned off this function to track app installation.
@IonesioJunior
docs(syftbox/apps/install.py): ADD docstrings to install.py functions
bdbc092
@yashgorana
potential fix for /tmp/ in macos
7fb9ffb
@madhavajay
Merge branch 'main' into feature/add_syftbox_install
b46b836
@madhavajay
Made app install less strict and tested against github_app_updater
493ebba 
- added support for full github urls
- added version command
- added debug command
- added just app
- added just install
madhavajay
madhavajay reviewed Oct 11, 2024
View reviewed changes
tests/client/app_manager/install_test.py


def test_clone_invalid_repository():
path = "InvalidUser/InvalidRepo"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is printing that you need to enter a github user because its not a valid url? Maybe it thinks its private?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot 2024-10-11 at 12 28 15 pm

madhavajay
madhavajay reviewed Oct 11, 2024
View reviewed changes
tests/client/app_manager/install_test.py


def test_load_app_config():
valid_json_config = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

iv made this optional for now and i think we can continue to iterate on how it would work

@madhavajay madhavajay marked this pull request as ready for review October 11, 2024 02:29
@madhavajay madhavajay self-requested a review October 11, 2024 02:29
madhavajay
madhavajay approved these changes Oct 11, 2024
View reviewed changes
Copy link
Collaborator

@madhavajay madhavajay left a comment

Choose a reason for hiding this comment

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

This is great work. Iv made some changes and tested it so we can use it to remove the default app copier from syftbox for now.

We can continue to iterate on it as we go.

@madhavajay madhavajay merged commit d30fd73 into main Oct 11, 2024
1 of 7 checks passed
@yashgorana yashgorana deleted the feature/add_syftbox_install branch October 11, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madhavajay madhavajay madhavajay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IonesioJunior @iamtrask @madhavajay @yashgorana