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

[FEATURE] great_expectations_contrib CLI tool #3909

Merged
merged 14 commits into from
Dec 29, 2021

Conversation

cdkini
Copy link
Member

@cdkini cdkini commented Dec 27, 2021

Please annotate your PR title to describe what the PR does, then give a brief bulleted description of your PR below. PR titles should begin with [BUGFIX], [FEATURE], [DOCS], or [MAINTENANCE]. If a new feature introduces breaking changes for the Great Expectations API or configuration files, please also add [BREAKING]. You can read about the tags in our contributor checklist.

Changes proposed in this pull request:

  • Introduce a CLI tool to aid contributors with custom packages
  • Link to custom Cookiecutter template

Definition of Done

Please delete options that are not relevant.

  • My code follows the Great Expectations style guide
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have run any local integration tests and made sure that nothing is broken.

Thank you for submitting!

@netlify
Copy link

netlify bot commented Dec 27, 2021

✔️ Deploy Preview for niobium-lead-7998 ready!

🔨 Explore the source changes: 978c0dc

🔍 Inspect the deploy log: https://app.netlify.com/sites/niobium-lead-7998/deploys/61cc81d61fee12000830e7d4

😎 Browse the preview: https://deploy-preview-3909--niobium-lead-7998.netlify.app

@github-actions
Copy link
Contributor

HOWDY! This is your friendly 🤖 CHANGELOG bot 🤖

Please don't forget to add a clear and succinct description of your change under the Develop header in docs_rtd/changelog.rst, if applicable. This will help us with the release process. See the Contribution checklist in the Great Expectations documentation for the type of labels to use!

Thank you!

@cdkini cdkini self-assigned this Dec 28, 2021
Copy link
Member

@abegong abegong left a comment

Choose a reason for hiding this comment

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

Quick review: https://www.loom.com/share/ffbda62cce7d4b1e8b5b26ba6f285326

Nonblocking issues to consider:

  • Let’s be cautious about defaults. I’ll defer to @talagluck and the DevRel team on this.
  • How can we strongly incline people to use our preferred *_expectations naming convention?
  • The cookiecutter template will need some changes and updates.
    • The directory structure needs updating
    • requirements.dev should have great_expectations as a dependency
    • Other changes to fit in with our CI + pypi deployment pipeline.

…tions into feature/great_expectations_contrib-cli-tool
Copy link
Contributor

@talagluck talagluck left a comment

Choose a reason for hiding this comment

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

Looks great overall! Just one question. Approving so as not to gate. Totally agree with Abe's comments, and happy to brainstorm on them, though it looks like that code is not in this PR - is that right?

from great_expectations_contrib.commands import check_cmd, init_cmd, publish_cmd

# The following link points to the repo where the Cookiecutter template is hosted
URL = "https://github.com/great-expectations/great-expectations-contrib-cookiecutter"
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ cookiecutter

return is_successful


def publish_to_pypi() -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Silly question - is there any security risk here? Just double checking that someone couldn't accidentally publish something that they didn't mean to publish.

Copy link
Member Author

Choose a reason for hiding this comment

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

great question! perhaps we want to get rid of this and just let the user go through the steps manually? it's easy enough to remove this if need be I'm really not too sure of the security implications

Copy link
Contributor

@kenwade4 kenwade4 left a comment

Choose a reason for hiding this comment

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

Aside from the inline comments, do we want to check that the user has a ~/.pypirc file before proceeding? Even though twine is a part of the requirements, I think it will fail to publish to pypi.org if there is not a ~/.pypirc file.

And to Abe's point about ensuring the project_name ends with "Expectations" and the project_slug ends with _expectations, do we want to inspect the string and append automatically if they don't have it?

contrib/cli/great_expectations_contrib/commands.py Outdated Show resolved Hide resolved
contrib/cli/great_expectations_contrib/commands.py Outdated Show resolved Hide resolved
contrib/cli/great_expectations_contrib/commands.py Outdated Show resolved Hide resolved
contrib/cli/great_expectations_contrib/commands.py Outdated Show resolved Hide resolved
@cdkini
Copy link
Member Author

cdkini commented Dec 28, 2021

@kenwade4 sorry just seeing your comments outside of the code.

I'm not too familiar with packaging so if you think .pypirc is the way to go, we can go that route! I've tested this with TestPyPi and it works without it but I'm happy to update.

I think I'll bring up this command during arch review to discuss the possible security implications @talagluck brought up. In regards to the naming conventions, Cookiecutter has input validation so we can add custom logic to reject a user's project if they don't append Expectations.

@kenwade4
Copy link
Contributor

If twine will prompt the user for their pypi creds if there is no pypirc file, that's probably good enough.

@cdkini cdkini enabled auto-merge (squash) December 29, 2021 15:42
@cdkini cdkini merged commit cabeb70 into develop Dec 29, 2021
@cdkini cdkini deleted the feature/great_expectations_contrib-cli-tool branch December 29, 2021 15:45
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