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.

Sign up for GitHub

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

Jump to bottom

Add support for GitHub Actions #4059

Closed
wants to merge 1 commit into from
Closed

Add support for GitHub Actions #4059

wants to merge 1 commit into from

Conversation

TobiGr
Copy link
Contributor

@TobiGr TobiGr commented Aug 2, 2020 •
edited
Loading

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

This enables GitHub Actions to work with PRs.
Build PRs pointing to dev or master branch and create a comment linking to the APK download page on success. Otherwise, the creator is notified (if enabled by them).
Example PR: TobiGr#7

ToDo

  • Check out the Git branch and do not use HEAD. Currently, all APKs have the id org.schabi.newpipe.HEAD and app name is "NewPipe HEAD"
  • Disable & Remove Travis integration
  • Require successful GitHub Action checks for merging a PR

Ideas

  • Do not comment a link to the workflow page when <!-- NO_APK_NOTIFICATION --> is in (preferably at the end of) the PR description.
  • Post a warning when a PR is approved, but the extractor repo is not set to TeamNewPipe/NewPipeExtractor

Fixes the following issue(s)

Agreement

@TobiGr TobiGr added the meta Related to the project but not strictly to code label Aug 2, 2020
@wb9688
Copy link
Contributor

wb9688 commented Aug 3, 2020

Something to check: if I change the workflow file in my PR, will it use the workflow file from my PR or from the branch it will merge into? If the first, it could be possible to change the workflow file to print secrets.GITHUB_TOKEN, which you obviously don't want.

@ghost
Copy link

ghost commented Aug 3, 2020

The grammar on that bot isn't perfect

@wb9688 wb9688 marked this pull request as draft August 3, 2020 12:38
@yausername
Copy link
Contributor

yausername commented Aug 3, 2020 •
edited
Loading

I opened a PR on @TobiGr 's fork, it runs the modified workflow file but fails to post comment.
The secrets.GITHUB_TOKEN given to forked PRs has read-only access to the upstream project. Hence posting comment fails.

TobiGr
TobiGr commented Aug 12, 2020
View reviewed changes
.github/workflows/build-pull-requests.yml
- name: Create comment linking to the artifact
uses: thollander/actions-comment-pull-request@1f25fabed60c3f141743c3a522529950a8fb2191
with:
message: 'The APK was build successfuly. You can find it here: https://github.com/TobiGr/NewPipe/actions/runs/${{github.run_id}}'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line needs to be changed

triallax
triallax reviewed Nov 8, 2020
View reviewed changes
.github/workflows/build-pull-requests.yml
jobs:
build:
name: Build project & generate APK
runs-on: ubuntu-18.04
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use ubuntu instead?

@B0pol
Copy link
Member

B0pol commented Nov 8, 2020

https://www.zdnet.com/article/google-to-github-times-up-this-unfixed-high-severity-security-bug-affects-developers

@opusforlife2
Copy link
Collaborator

https://www.zdnet.com/article/google-to-github-times-up-this-unfixed-high-severity-security-bug-affects-developers

I suppose we should wait, then.

@opusforlife2 opusforlife2 mentioned this pull request Nov 16, 2020
Closed
@FireMasterK
Copy link
Member

Having an action without the depreciated add-path or set-env commands should be safe according to https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/.

@XiangRongLin
Copy link
Collaborator

TravisCI changed their pricing last month, giving public repos only 1000min/month https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing#building-on-a-public-repositories-only.

I doubt it is enough with each build taking 6-7min, meaning ~150builds. Meaning the migration to GitHub actions would rise in importance

@TobiGr
Add support for GitHub Actions
79aed48 
Build PRs pointing to dev or master branch and create a comment linking to the APK download page.
@TobiGr
Copy link
Contributor Author

TobiGr commented Dec 8, 2020

@XiangRongLin Thanks for letting us know!

I just rebased my commit. I'll try to find a solution for the mentioned problems.

FireMasterK
FireMasterK reviewed Dec 9, 2020
View reviewed changes
.github/workflows/build-pull-requests.yml
- uses: actions/checkout@v1

- name: set up JDK 1.8
uses: actions/setup-java@v1.4.0
Copy link
Member

Choose a reason for hiding this comment

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

Change this to v1.4.3.
v.1.4.0 uses the vulnerable set-env command.

@FireMasterK FireMasterK mentioned this pull request Dec 9, 2020
Merged
5 tasks
@FireMasterK
Copy link
Member

I've created a pull request to just replace Travis: #5132

@TobiGr
Copy link
Contributor Author

TobiGr commented Dec 11, 2020

Closing in favour of #5132. See my comments regarding debug APKs in that thread.

@TobiGr TobiGr closed this Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@triallax triallax triallax left review comments

@FireMasterK FireMasterK FireMasterK left review comments

Assignees
No one assigned
Labels
meta Related to the project but not strictly to code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@TobiGr @wb9688 @yausername @B0pol @opusforlife2 @FireMasterK @XiangRongLin @triallax