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

Switch to GitHub Actions #211

Merged
merged 12 commits into from
Jul 9, 2020
Merged

Conversation

RJ722
Copy link
Contributor

@RJ722 RJ722 commented Jun 28, 2020

Description

Switch to GitHub Actions

@jendrikseipp, in order to report to coveralls, you'd need to add a secret COVERALLS_REPO_TOKEN. The repo-token can be found here: https://coveralls.io/github/jendrikseipp/vulture/settings

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation in the README file.
  • I have updated the documentation accordingly.
  • I have added an entry in CHANGELOG.md.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@RJ722
Copy link
Contributor Author

RJ722 commented Jun 28, 2020

https://github.community/t/run-a-github-action-on-pull-request-for-pr-opened-from-a-forked-repo/16054/2

Seems like inter-fork PRs introducing workflows don't trigger a build. @jendrikseipp can you pull this branch from my fork, and push as one of your branch, please?

@jendrikseipp
Copy link
Owner

I added a dummy workflow. Can you push to the branch to see if it builds now?

@RJ722
Copy link
Contributor Author

RJ722 commented Jun 29, 2020

Ah, the builds failed because it can't find the COVERALLS_REPO_TOKEN.

Feedback for @coverallsapp: It would be really nice to have pytest-coverage report supported for your GitHub Action. I am very unpleased with a lack of response from your end at coverallsapp/github-action#30.

@jendrikseipp
Copy link
Owner

Ah, the builds failed because it can't find the COVERALLS_REPO_TOKEN.

Do you need me to do something about this?

@RJ722
Copy link
Contributor Author

RJ722 commented Jun 29, 2020

Do you need me to do something about this?

Yes, you'd need to add a COVERALLS_REPO_TOKEN secret in GitHub (Repo Settings -> Secrets). The repo-token can be found here: https://coveralls.io/github/jendrikseipp/vulture/settings.

@jendrikseipp
Copy link
Owner

Done.

@RJ722
Copy link
Contributor Author

RJ722 commented Jun 29, 2020

Hmm, it still can't find it! :/

@jendrikseipp
Copy link
Owner

Maybe disable coveralls until the other tests pass?

@jendrikseipp
Copy link
Owner

The settings say "Secrets are not passed to workflows that are triggered by a pull request from a fork. (https://help.github.com/en/actions/configuring-and-managing-workflows/creating-and-storing-encrypted-secrets)".

@jendrikseipp
Copy link
Owner

Maybe we need to use the coveralls GitHub action? https://github.com/marketplace/actions/coveralls-github-action

@RJ722
Copy link
Contributor Author

RJ722 commented Jun 29, 2020

Maybe we need to use the coveralls GitHub action? https://github.com/marketplace/actions/coveralls-github-action

But, that doesn't support pytest-cov yet! We don't know whether they plan to support it at all, or not.

I'll remove the coveralls integration for now. We can add later!

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
@jendrikseipp
Copy link
Owner

I'll remove the coveralls integration for now. We can add later!

Sounds good.

@jendrikseipp
Copy link
Owner

The travis builds seem to have stopped working, so it would be nice to move this pull request forward :-) Is it ready for review?

@RJ722
Copy link
Contributor Author

RJ722 commented Jul 9, 2020

@jendrikseipp yep, this is good-to-go.

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

Looks good. Just some minor nitpicks :-) If you like, please add a changelog entry.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@jendrikseipp jendrikseipp merged commit 1b2ab61 into jendrikseipp:master Jul 9, 2020
@jendrikseipp
Copy link
Owner

Thanks, Rahul! Very nice that all tests are consolidated now!

@RJ722
Copy link
Contributor Author

RJ722 commented Jul 9, 2020

Yay! Here's a list of things I'd like to tackle next:

  • See if we can integrate the way we build wheels for Windows and *nix.
  • Track test coverage.
  • Use annotations to mark unused code in incoming PRs (e.g., https://github.com/qutebrowser/experiments/pull/23/files). I'd probably write another GitHub Action for this, which we can use in our CI system.

@jendrikseipp
Copy link
Owner

Nice! All three sound very good!

Can you elaborate on the third one? Are you planning to write a GitHub Action that anyone can add to their repositories to run Vulture? That sounds cool!

@RJ722
Copy link
Contributor Author

RJ722 commented Jul 9, 2020

Can you elaborate on the third one? Are you planning to write a GitHub Action that anyone can add to their repositories to run Vulture?

Yep, yep! Run Vulture, match the output against a regex and annotate the troubling lines.

All credit to @The-Compiler for the idea! :D

@jendrikseipp
Copy link
Owner

Nice! I guess you plan to use regexes because we can't use the API? Can't the Action use Python and use the API?

@The-Compiler
Copy link
Contributor

It's certainly possible to use the GitHub API to submit annotations, but registering a problem matcher file like:

{
  "problemMatcher": [
    {
      "severity": "warning",
      "pattern": [
        {
          "regexp": "^([^:]+):(\\d+): ([^(]+ \\(\\d+% confidence\\))$",
          "file": 1,
          "line": 2,
          "message": 3
        }
      ],
      "owner": "vulture"
    }
  ]
}

Seems like a much simpler solution (and doesn't require an API key and such)

@jendrikseipp
Copy link
Owner

Sounds good. Looking forward to seeing what you cook up :-)

@RJ722 RJ722 deleted the use-gh-actions branch July 11, 2022 13:23
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