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(sdk): add client methods to cli #7606

Merged

Conversation

connor-mccarthy
Copy link
Member

Description of your changes:
Adds the following functionality to the CLI per the V1 -> V2 CLI migration plan:

  • run archive
  • run unarchive
  • run delete
  • recurring-run enable
  • recurring-run disable
  • pipeline get-version
  • experiment unarchive

Also:

  • cleans up single/double quote inconsistency
  • improves informativeness of output for user

Checklist:

@google-oss-prow
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@connor-mccarthy
Copy link
Member Author

/test all

@connor-mccarthy connor-mccarthy marked this pull request as ready for review April 26, 2022 00:14
Copy link
Member

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

@click.argument('run-id')
@click.pass_context
def archive(ctx: click.Context, run_id: str):
"""Archive a run."""
Copy link
Member

Choose a reason for hiding this comment

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

nit:

The docstring should be descriptive-style ("""Fetches rows from a Bigtable.""") rather than imperative-style ("""Fetch rows from a Bigtable.""")

https://google.github.io/styleguide/pyguide.html#383-functions-and-methods

Copy link
Member Author

@connor-mccarthy connor-mccarthy Apr 27, 2022

Choose a reason for hiding this comment

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

Thanks and great point. These command docstrings serve a dual purpose: 1) they are standard docstrings and 2) they are the CLI help text.

I reviewed a few popular CLIs and it looks like most use an imperative style for help text:

  • git commit --help -> "Create a new commit containing..."
  • docker build --help -> "Build an image from a Dockerfile"
  • kubectl get pods --help -> "Display one or many resources"
  • gh pr list --help -> "List pull requests..."

So I went with imperative also. I could go either way and happy to make this change, but wanted to document the decision via a comment.

Note: git is actually somewhat inconsistent with this... git add --help -> "This command updates...".

Copy link
Member

Choose a reason for hiding this comment

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

  1. they are the CLI help text.

Didn't realize this. What you have is perfectly fine then. :) Thanks for the explanation.

@connor-mccarthy
Copy link
Member Author

/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: connor-mccarthy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow
Copy link

@connor-mccarthy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kubeflow-pipelines-samples-v2 ae767fe link unknown /test kubeflow-pipelines-samples-v2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@connor-mccarthy connor-mccarthy merged commit 027ac32 into kubeflow:master Apr 27, 2022
jagadeeshi2i pushed a commit to jagadeeshi2i/pipelines that referenced this pull request May 11, 2022
abaland pushed a commit to abaland/pipelines that referenced this pull request May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants