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

update Makefile, transition from Travis CI #1911

Merged
merged 5 commits into from
Jun 4, 2021
Merged

update Makefile, transition from Travis CI #1911

merged 5 commits into from
Jun 4, 2021

Conversation

ITProKyle
Copy link
Contributor

Summary

This PR is a follow up to my comment in #1904 where I volunteered to do some maintenance in the project and implement some QoL changes.

What Changed

Added

  • added a GitHub Actions workflow as a 1-to-1 replacement for the current Travis CI job
  • added various Makefile targets
    • help - print help dialog to the terminal that describes all the available targets
      target                         help
      ------                         ----
      help                           show this message
      fix-black                      automatically fix all black errors
      fix-isort                      automatically fix all isort errors
      lint                           run all linters
      lint-black                     run black
      lint-flake8                    run flake8
      lint-isort                     run isort
      test                           run tests
      
    • fix-black - while not implemented in the project, test had contained a call of black --check. this target would server to implement the changes highlighted by black.
    • fix-isort - while not implemented in the project, test had contained a call of isort --check. . this target would server to implement the changes highlighted by isort.
    • lint - run all linters (right now, only configured to forward to the lint-flake8 target as it is the only thing being run as a check currently). allows easy parody between what the GitHub Action is running and what can be run locally.
    • lint-black - split out from test, this uses black to check project code.
    • lint-flake8 - run the flake8 commands as they were in .travis.yml.
    • lint-isort - split out from test, this uses isort to check project code.

Changed

  • Makefile targets are now alphabetized (except for help which is listed first).
  • the test Makefile target no longer runs black and isort. it will only run tests as was defined in .travis.yml to allow easy parody between what the GitHub Action is running and what can be run locally.

Removed

  • removed .travis.yml as it will no longer be needed

@ITProKyle
Copy link
Contributor Author

FYI, the Action won't run since it's coming from a fork. I originally pushed the workflow without limiting the push event to the master branch to allow it to run once on my fork to show that it is working - https://github.com/ITProKyle/troposphere/actions/runs/904196552.

@ITProKyle ITProKyle mentioned this pull request Jun 3, 2021
pull_request:
push:
branches:
- master
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also include branch main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? This repo does not have a main branch and the default branch is master.

.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
clean:
rm -rf ${p39dir} troposphere.egg-info

lint: lint-flake8 ## run all linters
Copy link
Contributor

@michael-k michael-k Jun 4, 2021

Choose a reason for hiding this comment

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

Shouldn't this also run lint-black and lint-isort? Or the comment should be adjusted.

right now, only configured to forward to the lint-flake8 target as it is the only thing being run as a check currently

I don't think this restriction is necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this restriction is necessary

If I were to add the other targets here before the codebase is updated to pass both black and isort checks, the action I added will always fail. This target is being used as the main linting entry point for the workflow and to provide an entry point for local linting.

I plan to have a few follow-up PRs to this one that will address this concern by pairing it with updates to the code to pass these checks. I this them out, for now, to cut reduce the diffs for an easier review.

@michael-k
Copy link
Contributor

👍 for Travis CI → GitHub Actions

But instead of cementing the make approach, I'd prefer using tox more.

@ITProKyle
Copy link
Contributor Author

But instead of cementing the make approach, I'd prefer using tox more.

@michael-k - I tend to avoid tox as it does not play nice with a lot of other tools. Maybe there is a middle ground between using tox for its ability to handle running across different versions of python and makes ability to provide a low-effort set of commands to work within a project.

I can play around with it a bit or, it may be better suited for a separate PR.

Copy link
Member

@markpeek markpeek left a comment

Choose a reason for hiding this comment

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

LGTM. We can tweak per comments in follow up PR's.

@markpeek markpeek merged commit f5e01ae into cloudtools:master Jun 4, 2021
@markpeek
Copy link
Member

markpeek commented Jun 4, 2021

Thanks!

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.

3 participants