Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Build empty cookiecutters and run lint task during CI #1410

Merged
merged 10 commits into from
Feb 15, 2023

Conversation

flexponsive
Copy link
Contributor

@flexponsive flexponsive commented Feb 10, 2023

Related to #993

Feature Scope

Adds a Github Action to run basic end-to-end tests for the cookiecutters.

It builds empty cookiecutters from replay files, install its dependencies with poetry and runs some auto fixes on the generated code (flake8 & co). Finally, it runs the lint task as specified by the cookiecutter against itself. Additionally, a script which creates and lints in one step for developer convenience while working on cookiecutter templates.

This PR also includes a small shell script to build, install and lint a cookiecutter with a single command.

This PR also includes a few fixes (method signatures #1224, docstrings etc.) demanded by the linter for the case of a tap with REST stream type and API Key authentication method.

Relationship to test_cookiecutter.py

The SDK test suite already includes tests for building cookiecutters in test_cookiecutter.py, which runs flake8 and mypy. However, it does not use the same configuration for these tools as the template contains and also does not install dependencies with poetry so some errors are not picked up (e.g. method signatures mismatch). End-to-end tests can be a useful addition here.

Suggested GIthub Action config

Run the cookiecutter-e2e action only when cookiecutter files are changed; optionally on a regular schedule.


📚 Documentation preview 📚: https://meltano-sdk--1410.org.readthedocs.build/en/1410/

@tayloramurphy
Copy link
Collaborator

Thanks for the PR @flexponsive - looks like we've got some engineers assigned as reviewers who will get back to you likely sometime next week!

@edgarrmondragon
Copy link
Collaborator

Hey @flexponsive! Looks like flake8 is complaining. Let me know when this is ready to be reviewed 🙂

@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #1410 (e18a77c) into main (f953a0d) will not change coverage.
The diff coverage is n/a.

❗ Current head e18a77c differs from pull request most recent head 958f769. Consider uploading reports for the commit 958f769 to get more accurate results

@@           Coverage Diff           @@
##             main    #1410   +/-   ##
=======================================
  Coverage   85.39%   85.39%           
=======================================
  Files          54       54           
  Lines        4737     4737           
  Branches      808      808           
=======================================
  Hits         4045     4045           
  Misses        501      501           
  Partials      191      191           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@flexponsive
Copy link
Contributor Author

@edgarrmondragon I have updated the PR and description. Github does not show the results from the E2E-Tests of cookiecutters here in the PR, they are available here:

@aaronsteers
Copy link
Contributor

aaronsteers commented Feb 13, 2023

@flexponsive - First of all - thank you for creating this PR!

What do you think of adding a step to upload the generated projects using GitHub artifacts, if not already doing so?

https://github.com/actions/upload-artifact

- uses: actions/upload-artifact@v3
  with:
    name: my-artifact
    path: path/to/artifact/ # or path/to/artifact

Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

Hey @flexponsive. A few comments and suggestions. The bash script is good for an initial iteration of this but we should probably (not necessarily in this PR) port it to Nox since that's where we centralize dev tasks.

e2e-tests/cookiecutters/test_cookiecutter.sh Outdated Show resolved Hide resolved
.github/workflows/cookiecutter-e2e.yml Outdated Show resolved Hide resolved
.github/workflows/cookiecutter-e2e.yml Outdated Show resolved Hide resolved
.github/workflows/cookiecutter-e2e.yml Show resolved Hide resolved
e2e-tests/cookiecutters/test_cookiecutter.sh Outdated Show resolved Hide resolved
flexponsive and others added 2 commits February 14, 2023 09:13
@flexponsive flexponsive force-pushed the cookiecutter_ci branch 2 times, most recently from a8a1609 to dfb3af1 Compare February 14, 2023 08:46
@flexponsive
Copy link
Contributor Author

Thank you for the fast feedback and improvements to the PR!

@edgarrmondragon I accepted the suggestions and believe the issues you raised are fixed. I agree that it would be a good idea in the future to migrate from the bash script to tox but probably outside the scope of this PR.

@aaronsteers build artifact upload has been added too

Sample run: https://github.com/flexponsive/meltano-sdk/actions/runs/4172051621

@aaronsteers
Copy link
Contributor

build artifact upload has been added too

Nice! Thank you!

@edgarrmondragon
Copy link
Collaborator

Reviewing now...

@edgarrmondragon edgarrmondragon self-requested a review February 14, 2023 22:45
@edgarrmondragon edgarrmondragon self-requested a review February 14, 2023 22:46
@edgarrmondragon edgarrmondragon merged commit faf95bd into meltano:main Feb 15, 2023
@edgarrmondragon
Copy link
Collaborator

Thanks @flexponsive!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants