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

Add Github Actions Support #72

Merged
merged 3 commits into from Sep 3, 2020

Conversation

shibumi
Copy link
Collaborator

@shibumi shibumi commented Aug 20, 2020

Fixes issue #:
#62

Description of pull request:
This PR intents to remove travis and appveyor as CI and use Github Actions for everything, what we need.

Please verify and check that the pull request fulfills the following
requirements
:

  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@shibumi shibumi marked this pull request as draft August 20, 2020 16:28
@shibumi
Copy link
Collaborator Author

shibumi commented Aug 20, 2020

This is Work in Progress.. I am still playing around with Github Actions and Go.

EDIT: This will also add Tests on Mac OS

@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch 10 times, most recently from 82da6ca to 1f15442 Compare August 20, 2020 19:35
@shibumi
Copy link
Collaborator Author

shibumi commented Aug 20, 2020

Small update: MacOS and Linux is working, even coverage and everything works. What doesn't work is Windows and I don't know why yet.

on: [push, pull_request]
name: Test
jobs:
  test:
    strategy:
      matrix:
        go-version: [1.14.x]
        os: [ubuntu-latest, macos-latest, windows-latest]
    runs-on: ${{ matrix.os }}
    steps:
    - name: Install Go
      uses: actions/setup-go@v2
      with:
        go-version: ${{ matrix.go-version }}
    - name: Checkout code
      uses: actions/checkout@v2
    - name: Format Unix
      if: runner.os == 'Linux' || runner.os == 'MacOS'
      run: test -z $(go fmt ./...)
    - name: Format Windows
      if: runner.os == 'Windows'
      run: if (gofmt -l ./...) { Write-Error "Use gofmt" }
    - name: Test
      run: go test -race -covermode atomic -coverprofile=profile.cov ./...
    - name: Send coverage
      env:
        COVERALLS_TOKEN: ${{ secrets.GITHUB_TOKEN }}
      run: |
        GO111MODULE=off go get github.com/mattn/goveralls
        $(go env GOPATH)/bin/goveralls -coverprofile=profile.cov -service=github

This is our current workflow file. If you want to see how this look like, click here: https://github.com/shibumi/in-toto-golang/runs/1009326668?check_suite_focus=true

@shibumi
Copy link
Collaborator Author

shibumi commented Aug 20, 2020

A few open questions:

  • Right now we are calculating the coverage three times.. This is not necessary.. so this might should be in an own job.
  • Testing, however, should still be run on all three OS (I had issues with paths in the past, Linux has been fine, Windows has been not)
  • I don't know if the 'barrier' works with the current goveralls setup.. the runner is printing an URL and that URL works.. so we can see the coverage there.

I also thought about testing with different Go versions, but our strict dependency on Go 1.15 would make this pretty lonely right now.

@lukpueh
Copy link
Member

lukpueh commented Aug 21, 2020

A few open questions:

  • Right now we are calculating the coverage three times.. This is not necessary.. so this might should be in an own job.

I think it's fine to calc coverage for each job, because different platforms could have different coverage if there's platform-dependent case handling. And IIRC coveralls.io aggregates multiple results from one build.

  • Testing, however, should still be run on all three OS (I had issues with paths in the past, Linux has been fine, Windows has been not)

👍

  • I don't know if the 'barrier' works with the current goveralls setup.. the runner is printing an URL and that URL works.. so we can see the coverage there.

'barrier'?

I also thought about testing with different Go versions, but our strict dependency on Go 1.15 would make this pretty lonely right now.

Let's add tests for newer versions as they get released. :)

@shibumi
Copy link
Collaborator Author

shibumi commented Aug 24, 2020

'barrier'?

What I mean with 'barrier', is the PR-blocker. For example:"Coverage decreased more than 1.0%, merging is blocked".
I have no idea if this works with 3 different coverages.

@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch 8 times, most recently from 7e265ba to a5b27e2 Compare August 25, 2020 01:21
@lukpueh
Copy link
Member

lukpueh commented Aug 25, 2020

What I mean with 'barrier', is the PR-blocker. For example:"Coverage decreased more than 1.0%, merging is blocked".
I have no idea if this works with 3 different coverages.

I see. Good question. I think it sends the aggregate value. But I'm not sure. It does seem to work for python-(in-toto) (see e.g. https://coveralls.io/builds/26585333, where we have one runaway value that affects the overall result).

@shibumi
Copy link
Collaborator Author

shibumi commented Aug 25, 2020

I see. Good question. I think it sends the aggregate value. But I'm not sure. It does seem to work for python-(in-toto) (see e.g. https://coveralls.io/builds/26585333, where we have one runaway value that affects the overall result).

We could just merge as soon I have fixed that windows build and see if the PR-blocker will appear(?) or I test this on my own repository with different branches.

@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch 2 times, most recently from 9192dfd to 2236c3e Compare August 26, 2020 20:03
@shibumi shibumi closed this Aug 26, 2020
@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch from 2236c3e to f2cca63 Compare August 26, 2020 20:09
@shibumi shibumi reopened this Aug 26, 2020
@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch 3 times, most recently from e20ff51 to d8a6bb8 Compare August 26, 2020 21:22
With this commit we are removing Travis-Ci and Appveyor. Furthermore
it introduces a .gitattributes file in the test directory for making sure,
that the test files are in the right format on windows.
@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch from d8a6bb8 to 18a4116 Compare August 26, 2020 21:23
@shibumi
Copy link
Collaborator Author

shibumi commented Aug 26, 2020

@lukpueh Summary for you, because I know you are rare of time :)

  • Github Actions works on Windows, Ubuntu and MacOS now
  • But we only run format + send coverage on Ubuntu right now. I still have a few path problems on Windows I don't get fixed. For example on Windows if I call gofmt I get permission denied errors + sending coverage fails, too. We could enable format + coverage for MacOS, too. What's your opinion?
  • First I was going for a .gitattributes file in test/data, the latest commit (04b13d5) removes this, via replacing all "\r\n" in a file in the RecordArtifact step via "\n". So the .gitattributes file is now longer needed.

If we merge this now, we can be sure for later commits, that it works on all plattforms. However, I don't know if we might have similar issues with reading links(?). Are we unmarshalling the links directly?

@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch from 04b13d5 to b184909 Compare August 27, 2020 21:21
@shibumi
Copy link
Collaborator Author

shibumi commented Aug 28, 2020

The current Github Actions Output can be found here: https://github.com/shibumi/in-toto-golang/runs/1038690604?check_suite_focus=true

I guess, this is ready to merge.

@shibumi shibumi marked this pull request as ready for review August 28, 2020 14:35
@shibumi
Copy link
Collaborator Author

shibumi commented Aug 28, 2020

We might want to remove the appveyor CI afterwards, via the appveyor controlpanel.

in_toto/runlib.go Outdated Show resolved Hide resolved
This adds a bytes.ReplaceAll directive in the RecordArtifact function,
that replaces all CRLF line separators with LF line separators.
We see LF line separators as our normalized format.
@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch from b184909 to c5cb513 Compare September 1, 2020 16:59
@shibumi
Copy link
Collaborator Author

shibumi commented Sep 1, 2020

@lukpueh I have added your comment :) should be fine now.

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @shibumi!

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Oh, just realised that we will also want to remove the badges in README.md, and add new ones if they exist.

@shibumi
Copy link
Collaborator Author

shibumi commented Sep 2, 2020

Oh, just realised that we will also want to remove the badges in README.md, and add new ones if they exist.

this is a good idea. I will add them.

README.md Outdated
@@ -1,5 +1,5 @@
# In-toto Go implementation
[![Build Status](https://travis-ci.com/in-toto/in-toto-golang.svg?branch=master)](https://travis-ci.com/in-toto/in-toto-golang) [![Coverage Status](https://coveralls.io/repos/github/in-toto/in-toto-golang/badge.svg)](https://coveralls.io/github/in-toto/in-toto-golang) [![Build status](https://ci.appveyor.com/api/projects/status/n45pmpso0t5b40vk?svg=true)](https://ci.appveyor.com/project/in-toto/in-toto-golang)
![build](https://github.com/in-toto/in-toto-golang/workflows/build/badge.svg) [![Coverage Status](https://coveralls.io/repos/github/in-toto/in-toto-golang/badge.svg)](https://coveralls.io/github/in-toto/in-toto-golang)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the first badge a link to the GitHub build page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, sure.. wait a second

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lukpueh solved, but we can only link to the build workflow overview page. A link to the latest build is not possible (Github is working on this)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the fix and headsup!

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we have to first activate something for this to work on the upstream repo?

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge and see. :)

We need to rename the job in the Github Actions to properly display the word "build" in the README.md
@shibumi shibumi force-pushed the shibumi/switch-to-github-actions branch from 71d33bd to acb2a48 Compare September 3, 2020 10:55
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.

None yet

2 participants