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

Custom actions #700

Merged
merged 4 commits into from Aug 12, 2020
Merged

Custom actions #700

merged 4 commits into from Aug 12, 2020

Conversation

mlucool
Copy link
Contributor

@mlucool mlucool commented Jul 21, 2020

Fixes: #584

This adds the ability for users to set a custom class to take actions. As of now, we only have a post_init action, but this can be trivially extended to give more power to end users who have non-generic needs.

Example ~/.jupyter/jupyter_notebook_config.json:

  "JupyterLabGitConfig": {
      "custom_git_actions_class": "org.dir.MyClass"
  }

@mlucool
Copy link
Contributor Author

mlucool commented Jul 21, 2020

I'm add to add documentation to the README and test cases if this is the right design. Thought I'd bring this up for feedback first

@fcollonval
Copy link
Member

@mlucool Thanks for this PR. Using server configuration is safe enough - managed installation will keep control 😉

Regarding the implementation, I suggest to not be as generic for two reasons:

  • We don't want to deal with complex custom Python code plugged-in
  • We want to keep some control over those side-effects - like be able to implement a timeout feature around them.

So I propose to use the following configuration to a list of subprocess commands to be executed in the repository folder:

{
  "JupyterLabGit": {
    "actions": {
      "postInit": [{
        "command": Array<string>, // subprocess command
        "args": Dict[string, string]  // subprocess args
      }]
    }
  }
}

Some comments on the schema:

  • Use JupyterLabGit shorter name - we know that we are speaking about configuration
  • Add a subentry actions so this will allow to organize the configuration parameters - if it grows in the future
  • Add actions subentry with clear name about when it is gonna be executed (e.g. you could add a postClone entry)

What do you think?

To make it works easily with Configurable, I suggest to use AutoInstance implemented in jupyter-project.

@mlucool
Copy link
Contributor Author

mlucool commented Jul 23, 2020

Regarding the implementation, I suggest to not be as generic for two reasons:

I don't totally agree, but I'm happy to oblige. One key reason is that the API is very hard to establish - but maybe we just promise to give the script nothing and we call it in the right cwd. I also find in process error handling mostly easier. Still, we ca go with arbitary shell commands only, but I think we just support anything a user puts and use shlex to break it up or use the shell=True ( i.e. '/my/script.py --foo' is a valid command to pass).

Some comments on the schema:

These all seem reasonable

@fcollonval
Copy link
Member

but I think we just support anything a user puts and use shlex to break it up or use the shell=True ( i.e. '/my/script.py --foo' is a valid command to pass).

Good point, this will ease the setting.

@mlucool
Copy link
Contributor Author

mlucool commented Jul 24, 2020

@fcollonval updated. Any more feedback or I will finish cleaning this up (more doc, seeing if its easy enough to test). I can add this to places besides init (e.g. clone) as well.

@mlucool
Copy link
Contributor Author

mlucool commented Jul 30, 2020

@fcollonval did you have further feedback?

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @mlucool

I left a few questions in your proposal.

For the test, could you test also a failure of execute with a non-zero code and with the emission of an exception? This is quite critical and will help use ensuring robustness of the extension.

jupyterlab_git/__init__.py Outdated Show resolved Hide resolved
# After any failure, stop
if code != 0:
break
return code, stdout, stderr
Copy link
Member

Choose a reason for hiding this comment

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

Opened question: Should the stdout be concatenate for all actions?

)
if code == 0:
code, _, error = await self._maybe_run_actions('post_init', cwd)
Copy link
Member

Choose a reason for hiding this comment

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

Opened question: Should stdout logged to help debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have any examples of logging? For now, I sent all data (stdout/stderr/code/command) back to the front end where we can later show whatever we'd like to the user.

jupyterlab_git/git.py Outdated Show resolved Hide resolved
@mlucool
Copy link
Contributor Author

mlucool commented Aug 3, 2020

@fcollonval this is ready for re-review.

For the test, could you test also a failure of execute with a non-zero code and with the emission of an exception? This is quite critical and will help use ensuring robustness of the extension.

Tested no post_init, post_init that works, and post_init that fails

@mlucool mlucool requested a review from fcollonval August 3, 2020 20:06
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

@mlucool Thanks for the addition of the tests. I saw one error. And then we should be good to go.

About the logging, you can drop it for now - I'm looking into this without the need to pass a logger object from the notebook application.

jupyterlab_git/git.py Outdated Show resolved Hide resolved
jupyterlab_git/tests/test_init.py Show resolved Hide resolved
@fcollonval fcollonval added this to the 0.21.0 milestone Aug 8, 2020
@fcollonval fcollonval mentioned this pull request Aug 9, 2020
@mlucool
Copy link
Contributor Author

mlucool commented Aug 11, 2020

@fcollonval I believe all review feedback is taken care of

Copy link
Member

@fcollonval fcollonval 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 a lot @mlucool

@fcollonval fcollonval merged commit 06326ae into jupyterlab:master Aug 12, 2020
@fcollonval
Copy link
Member

@meeseeksdev backport to 0.11.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab-git that referenced this pull request Aug 12, 2020
fcollonval added a commit that referenced this pull request Aug 12, 2020
…on-0.11.x

Backport PR #700 on branch 0.11.x (Custom actions)
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.

Git Init Menu
2 participants