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

Set up all CI to use GH actions #1443

Conversation

ben-alkov
Copy link
Member

Signed-off-by: Ben Alkov ben.alkov@redhat.com

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • JSON/YAML configuration changes are updated in the relevant schema
  • Changes to metadata also update the documentation for the metadata
  • Pull request has a link to an osbs-docs PR for user documentation updates
  • New feature can be disabled from a configuration file

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/containerbuildsystem-atomic-reactor-1443
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@ben-alkov ben-alkov force-pushed the investigate-ci-using-gh-workflow branch 2 times, most recently from b1e42ef to aee2018 Compare April 29, 2020 18:25
@ben-alkov ben-alkov changed the title Set up all CI to use GH actions [WIP] Set up all CI to use GH actions Apr 29, 2020
@ben-alkov ben-alkov changed the title [WIP] Set up all CI to use GH actions WIP Set up all CI to use GH actions Apr 29, 2020
@ben-alkov ben-alkov marked this pull request as draft April 29, 2020 18:32
@ben-alkov ben-alkov changed the title WIP Set up all CI to use GH actions Set up all CI to use GH actions Apr 29, 2020
@MartinBasti
Copy link
Contributor

What is status of this PR? Can it be closed?

@stickler-ci
Copy link

stickler-ci bot commented Jun 2, 2020

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

@ben-alkov
Copy link
Member Author

ben-alkov commented Jun 2, 2020

What is status of this PR? Can it be closed?

@MartinBasti

This was meant as a P.O.C., and also to gauge interest, but I never got any feedback - if we're interested in moving forward with this, there's very little work left; this PR is practically done.

@MartinBasti; What should I do with this? Are we interested in pursuing it? If so , I'll open it for reviews.

@MartinBasti
Copy link
Contributor

I'd like move to GH actions then

@ben-alkov ben-alkov marked this pull request as ready for review June 4, 2020 19:16
@ben-alkov
Copy link
Member Author

ben-alkov commented Jun 4, 2020

@athos-ribeiro, @chmeliik, @lcarva, @MartinBasti, @rcerven, @twaugh; You can see what a run looks like here

Obviously, this could be extended to osbs-client, koji-c, d-f-p etc.

@ben-alkov ben-alkov requested review from lcarva and twaugh June 4, 2020 19:21
@MartinBasti
Copy link
Contributor

So we will not have such nice link to coveralls web to see details of coverage?

Why stickler is not there anymore?

@ben-alkov
Copy link
Member Author

ben-alkov commented Jun 4, 2020

So we will not have such nice link to coveralls web to see details of coverage?

Yeah, that's a little bit of a bummer. Unfortunately, Coveralls is very hard to work with (and Codecov is worse!)... More work may be necessary to tweak the Coveralls action so that we get info "up front" rather than having to dig for it (but n.b. that I don't know that this is even possible).

EDIT: Ah, I remembered. Using Coveralls with a Python project is a total kludge and a hack. If you're using a supported language (JS, whatever) your action's "Details" link points out to Coveralls.io

Why stickler is not there anymore?

We can easily add flake8 and shellcheck GH actions

.coveragerc Outdated Show resolved Hide resolved
@ben-alkov ben-alkov force-pushed the investigate-ci-using-gh-workflow branch from 94d1a1d to 8676d2c Compare June 5, 2020 13:37
@ben-alkov
Copy link
Member Author

So we will not have such nice link to coveralls web to see details of coverage?

@MartinBasti; OK, got it.

  • -- pytest --cov=atomic_reactor --cov-report= --color=yes tests
  • ++ coverage run --source=atomic_reactor -m pytest --color=yes tests

Now, if you click on the "Details" link in the "CI box", you'll go directly to Coveralls.io. This also fixed some odd issues I saw at Coveralls with formatting etc.

@ben-alkov ben-alkov force-pushed the investigate-ci-using-gh-workflow branch 5 times, most recently from d98b639 to 6aa54b1 Compare June 5, 2020 20:49
@ben-alkov ben-alkov force-pushed the investigate-ci-using-gh-workflow branch from 6aa54b1 to af14eab Compare June 5, 2020 21:02
@ben-alkov
Copy link
Member Author

Added flake8 and shellcheck actions.

N.B. LGTM doesn't operate on forks - I have to take it on faith that it will continue to work if this is merged to mainline, but I have no reason to doubt that it will.

@ben-alkov ben-alkov force-pushed the investigate-ci-using-gh-workflow branch 3 times, most recently from 3d3feb4 to 56533cc Compare June 11, 2020 17:58
@athos-ribeiro
Copy link
Contributor

Looks good. Thanks, Ben.

Any objections on taking this path for all repos then?

@chmeliik
Copy link
Contributor

Looks good. Thanks, Ben.

Any objections on taking this path for all repos then?

I'm all for it, I'd merge this and if it works well, we can update other repos.

@MartinBasti
Copy link
Contributor

Is code compatible with flake8? I liked stickler because it points to issues only in new changes. Why we cannot stay with stickler?

@ben-alkov
Copy link
Member Author

Is code compatible with flake8? I liked stickler because it points to issues only in new changes. Why we cannot stay with stickler?

@MartinBasti

I fixed the tiny handful of space/newline issues flake8 was moaning about.

The problems I have with Stickler:

  • Black box
  • Unknown "default" rule config - it's not flake8's defaults (AFAICT), but I see no documentation of it anywhere
  • Difficult to configure

Part of my goal here was to remove 3rd-party dependencies:

  • Black boxes
  • Unpredictable SLAs (esp. recently)
  • No guarantees that free plans won't be changed/limited/dropped altogether
  • You get what they give you - little control over what's being done
    • For example, if we want to start running flake8-bugbear, or comprehensions -- plugins are a paid-only option for Stickler. They'd be a one-line change in the Dockerfile for the 'flake8' action

I even started out with a multi-linter made by someone else, then once I saw how straightforward it was to create actions - I just made my own:

  • We have complete control over every aspect of the action; the code can be as minimal or complex as we want. A fully-functional flake8 action is only a few lines of code
  • We can have e.g. a 'containerbuildsystems/actions' repo, with separate dirs for each action (see ben-alkov/actions for an example)
    • We can reuse our custom actions in all of our repos
    • It should be possible to have custom configs per-repo, if needed, while maintaining the same base action source
  • Since actions always run parallel to each other, there's really no need to have anything other than pytests in test.sh - bandit won't slow down the pytest runs; mdlint won't hold up pylint runs, etc

Honestly, this is probably better reviewed in my personal repo's PR, where you can see the results of the actions in place. The only remaining issue I have there is @(&#^$ shellcheck...

The coup de grâce, for me: if any action fails for whatever reason, instead of having to re-run all of the time and resource-intensive pytests, just to e.g. re-run 'bandit', it's possible to re-trigger each action separately, manually, without having to re-push. This saves time, energy, bandwidth, frustration...

@chmeliik
Copy link
Contributor

Honestly, this is probably better reviewed in my personal repo's PR, where you can see the results of the actions in place. The only remaining issue I have there is @(&#^$ shellcheck...

Ah, so the schellcheck action would fail if merged right? It might be nice to have an option to limit shellcheck to specific files, like only test.sh for example

@ben-alkov
Copy link
Member Author

ben-alkov commented Jun 24, 2020

@chmeliik

Ah, so the schellcheck action would fail if merged right? It might be nice to have an option to limit shellcheck to specific files, like only test.sh for example

Totally doable

EDIT: Done

@ben-alkov ben-alkov closed this Jun 24, 2020
@ben-alkov ben-alkov reopened this Jun 24, 2020
@ben-alkov ben-alkov force-pushed the investigate-ci-using-gh-workflow branch 2 times, most recently from b96e161 to 5821130 Compare June 24, 2020 15:21
@ben-alkov
Copy link
Member Author

Ah, so the shellcheck action would fail if merged right?

Nope! After configuring to only check 'test.sh', and silencing two complaints therein, everything is green 🚀

Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
@ben-alkov ben-alkov force-pushed the investigate-ci-using-gh-workflow branch from 5821130 to cfdef6d Compare June 24, 2020 18:02
@ben-alkov ben-alkov merged commit d30a67c into containerbuildsystem:master Jun 24, 2020
@ktdreyer
Copy link
Contributor

ktdreyer commented Jul 1, 2020

There are a lot of hard-coded Fedora versions in here, eg "30" and "29" have been EOL for some time now.

Someone or something needs to watch the Fedora versions in https://github.com/containerbuildsystem/actions as well to make sure those do not go EOL.

@ben-alkov ben-alkov deleted the investigate-ci-using-gh-workflow branch July 10, 2020 17:48
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.

5 participants