openwisp-utils icon indicating copy to clipboard operation
openwisp-utils copied to clipboard

[qa] Does commit check work on github actions?

Open nemesifier opened this issue 4 years ago • 4 comments
trafficstars

Looking at the current commit message check, I see travis being mentioned a few times:

https://github.com/openwisp/openwisp-utils/blob/30ad216d6e12a7bf4683f131967e763dbcc8363c/openwisp-qa-check#L104-L106

https://github.com/openwisp/openwisp-utils/blob/30ad216d6e12a7bf4683f131967e763dbcc8363c/openwisp-qa-check#L143

I wonder how this impacts on the builds on github actions. @pandafy @atb00ker any suggestion?

nemesifier avatar Dec 01 '20 15:12 nemesifier

The commit check works on GitHub Actions because @atb00ker made commitcheck run every time in https://github.com/openwisp/openwisp-utils/commit/fb3ffcc2c3c42dd065d93248d87433433de31421. Running on GitHub Actions is equivalent to running on a local machine for the script.

pandafy avatar Dec 01 '20 15:12 pandafy

@pandafy However,

  if [ "$TRAVIS" = true ] && [ -z "$TRAVIS_PULL_REQUEST" ]; then
     echo "SKIPPED: Commit message check skipped!"
  fi

this condition was added because when you create a merge commit, it fails on master, so we would need an equivalent condition to don't run on openwisp-[repo]/master! :smile:

Also, I think we shouldn't remove the Travis conditions but just add the GitHub actions conditions so that travis remains usable! (Still required for docker-openwisp, moving that one is hard because building docker images in CI is 💰 and not everyone allows it, I know it's not easy on Gitlab! 😅 )

atb00ker avatar Dec 01 '20 17:12 atb00ker

@atb00ker if I am not wrong, cases like merge commits are handled here https://github.com/openwisp/openwisp-utils/blob/7d9dfa1be17e5bb8893c0d05667dc8cd913271f8/openwisp_utils/qa.py#L92-L103

devkapilbansal avatar Apr 13 '21 05:04 devkapilbansal

@devkapilbansal this is rather an old comment and the process has been changed since, please ignore the comment if it is not valid.

Bottom line is we need to ensure the tests run when:

  • We test on CI
  • We test on Local

And they don't fail because of merge commits etc! :smile:

atb00ker avatar Apr 15 '21 17:04 atb00ker

Hello, Is there anyone working on this issue if not then can you assign this issue to Me?

Vihar214 avatar Dec 04 '22 17:12 Vihar214

Hi @Vihar214 ,

You don't need to ask anyone to assign it, you can just work on the issue and create a PR. Someone should look at it soon!

P.S: This ticket is rather old, if it is outdated and not required anymore, please share your findings. 😄

atb00ker avatar Dec 06 '22 21:12 atb00ker