sdnfv / openNetVM

A high performance container-based NFV platform from GW and UCR.
http://sdnfv.github.io/onvm/
Other
263 stars 135 forks source link

GitHub actions #196

Closed bdevierno1 closed 4 years ago

bdevierno1 commented 4 years ago

Implement Github Actions

Automates part of our CI process with Github Actions. It checks if users have created a pr to the correct branch, performs linter on modified code. It lints files with all of the following: gwclint.py, shellcheck, cppcheck, pylint. More detailed explanations can be found in the style/github-action-style.md

Usage:

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes
New functionality ๐Ÿ‘
New NF/onvm_mgr args
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

TODO before merging :

Test Plan:

Review:

Special thanks to co-author @kevindweb who this would not be possible without.

onvm commented 4 years ago

In response to PR creation

CI Message

Your results will arrive shortly

kevindweb commented 4 years ago

great job! ๐Ÿ”ฅ Code looks clean, much better than the current CI linter process! Once this is merged, I'll remove the branch check and linter from CI. We probably won't have much use for unatuhorized CI runs after GitHub checks are created but that's ok!

kevindweb commented 4 years ago

Just gave you access to run CI @bdevierno1 . Any ideas of how we can test this PR Ben? Could you provide a link to your test cases in your repository as well?

bdevierno1 commented 4 years ago

Just gave you access to run CI @bdevierno1 . Any ideas of how we can test this PR Ben? Could you provide a link to your test cases in your repository as well?

@kevindweb @dennisafa If you would like to test this within your own repository you would have to just copy over the two files and update line 12 of get-user-lint.sh to your own repo name. Create a pr from a new branch to your master/develop. If you click on Actions you will see a workflow would have started.

I have three tests below I performed in my own repo.

You can view these three pull request here. https://github.com/bdevierno1/openNetVM/pulls

kevindweb commented 4 years ago

Just saw this in your repo @bdevierno1 https://github.com/bdevierno1/openNetVM/pull/15/checks?check_run_id=520348316

Check out the last few lines of User's Lint. This is not a good sign I don't think

grep: file2.txt: No such file or directory

Seems like that's the only reason it passed because when the grep happens, it doesn't put anything into diff.txt. It doesn't seem like something we want in the Github check success.

bdevierno1 commented 4 years ago

https://github.com/bdevierno1/openNetVM/pull/15/checks?check_run_id=520348316

@kevindweb Thanks ๐Ÿ‘ The issue was when new files were created, the file was could not be lint by the master. So if u made a pr with only new files, this would result in that issue as only stderr will write to file2. I added a line that has resolved this. Re-did all the tests and my previous comment has been updated with the new links.

kevindweb commented 4 years ago

Yay! People can look at Ben's forked repo if they want to see his testing. I think the code looks good. Ben and I have talked a lot about the updates he just pushed. The code pushed in the get-user-lint.py file is an updated version of the current linting that will be removed from CI.

Ben did a good job abstracting out the code for each of the different linters (bash, python, c, etc). Because of this, in the future, we will be able to add any new linter or tool with significant ease.

bdevierno1 commented 4 years ago

Made a bunch of nit suggestions. Here is what I use in vim to see the whitespace

:highlight ExtraWhitespace ctermbg=red guibg=red
:match ExtraWhitespace /\s\+$/

I also feel that we should change the name from get-user-lint.sh to something more specific to C code, like get-clint.sh (maybe something more clever). As we add *.sh and *.py linter actions, we'll probably put those scripts in the /style directory as well. Keeping them unambiguous will be helpful.

Considering we have now added four different types of static analysis, get-clint is now artifact. Please let me know if anyone has any suggestions for the nomenclature.

twood02 commented 4 years ago

@bdevierno1 - Check if we can easily adjust the git diff line to make it include changes from uncommitted files (possibly just remove ...HEAD)

bdevierno1 commented 4 years ago

Since last review installation script has been added, updated such that users can test w/o having to commit. Note for new files users still have to git add filename. I have also updated how shellcheck parses the lint results in run-lint.py. Semi related to https://github.com/sdnfv/openNetVM/issues/161. Pylint will throw syntax errors for python2 scripts. This is especially noticeable for files with print statements w/o parantheses. Since we are using pylint-2.4 which relies on python3, it will exit once it reaches this line and not parse the rest of the file.

kevindweb commented 4 years ago

@onvm final test

onvm commented 4 years ago

@onvm final test

CI Message

Your results will arrive shortly