- 
  Notifications
 You must be signed in to change notification settings 
- Fork 24
Change action from docker to composite #83
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good proof of concept. I love the faster run times!
The workflow log commands don't seem to be getting captured by the runner. This affects the file-annotations feature and the output variable. I'll have to look into why it is broken, but it might have something to do with actions/runner#1742 (above my knowledge set).
I think I can retry using the REST API for file-annotations, but the output variable might have to become the cpp_linter.run.main()'s exit code (not preferable but that's how pylint works).
Note: Instead of retag the current tag still to v1, I'd like to add the v2 tag after this PR and #81 (two significant changes) are merged
Codecov Report
❗ No coverage uploaded for pull request base (
main@df06936). Click here to learn what that means.
The diff coverage isn/a.
❗ Current head 92298bd differs from pull request most recent head 0070d02. Consider uploading reports for the commit 0070d02 to get more accurate results
@@ Coverage Diff @@ ## main #83 +/- ## ======================================= Coverage ? 80.47% ======================================= Files ? 6 Lines ? 630 Branches ? 0 ======================================= Hits ? 507 Misses ? 123 Partials ? 0
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
I think we could clean the Python source code in this PR or do you want to create another RP to do the cleanup? After cpp-linter/cpp-linter#1 is done, then this PR can be merged.
I don't want to merge this until I can verify that the file annotations and output variable are working correctly. I could remove the python src in this PR (or do it in another PR - I don't care either way), but we should start using this PR to test the cpp-linter/cpp-linter#1 after it is merged.
This isn't going to be a quick fix. I think it needs more research/time to get done correctly.
I could remove the python src in this PR
Sure, thank you!
I can verify that the file annotations and output variable are working correctly
If the actions run successfully in test-cpp-linter-action repo (may include unit test passed), is it basically to prove that they should work correctly?
Making this change will either succeed or fail, I guess there shouldn't be a specific function that doesn't work.
 
 
 This comment was marked as outdated.
 
 
 
 
 This comment was marked as outdated.
I released the cpp-linter pkg to PyPI (using the tag v1.4.6) and made the workflow install the pkg from PyPi (not of from the git repo). This resulted in increased runtime; it took about 2 seconds to install the composite action's dependencies!
I also changed the file-annotations option to false, so I could see if they work. Surprisingly they do work. However, the output variable does not, and I think it is getting overridden by the composite step that sets it to nothing:
# there should be a value at the end of this string run: echo "::set-output name=checks-failed::"
aae0cfe to
 2c09969  
 Compare
 
 Well, I got the output var working as an exit code, but this will definitely require a v2.0.0 tag because it is a rather breaking change... See this commit's diff and this github doc for reference
I replaced the action's output variable with a new input option called quiet-fail.
- If quiet-failis false, then the action will return an unsuccessful exit code (1) but only if checks failed.
- If quiet-failis true, then the action will return a successful exit code (0) despite whether or not checks failed.
I intend to make this new input into a CLI option for cpp-linter pkg. The default value will be set to true, so it shouldn't break any workflows that are currently ignoring the output variable.
I think this PR is ready for review.
I'm already getting confused which branch is for which version. Can we rename the newer branch v2.x?
I also need to add a warning to the setup.py about using the master (削除) main (削除ここまで) branch during pip install.
Sorry, I should clarify what I did about switching branches:
- Released the last v1release, the tag isv1.5.0from the master branch
- Create main branch from master as our future branch
- Change the default branch from master to main
- Locked master to prevent new changes.
I'd like to merge all the future changes to the main branch and release v2from the main branch.
Can we rename the newer branch v2.x?
Starting from the main branch is our v2.x, do you think we still need it?
I understand all that. I'm just used to only seeing a master or a main branch for a repo (not both).
Years ago (in a galaxy far far away) before Microsoft bought out Github, the default branch name was master for a newly created repo. Since the buyout, the name for the default branch changed to main. Most of my repos still use master (because I've been at this SCM stuff for a while now), but some of my repos now use main. Having 2 branches for this 1 repo that use master and main is a bit confusing for me - I can only imagine what onlookers think when browsing the repo looking to contribute.
I'm just used to only seeing a master or a main branch for a repo (not both).
Me too 😃
I still want to use main to keep the main branch of the repository under this organization unified (main), which should be more convenient for us to use main than v2.x.
I guess people who will browse our branch may only be those who may want to contribute or learn, we can add a contribute.md to introduce the branches and why we keep the master branch because there are still users using it this branch.
Of course, I'm not against using v2.x as the main branch. Your thoughts?
we can add a contribute.md to introduce the branches and why we keep the master branch because there are still users using it this branch.
I like the CONTRIBUTING.md idea. I'm kind of surprised I hadn't thought about that yet; I typically add one to all my fresh/on-boarding projects.
I still want to use main to keep the main branch of the repository under this organization unified (main), which should be more convenient for us to use main than v2.x.
I can't argue with that.
I suppose I'll get used to using the main branch rather than the master branch. It might take some time for personal adjustment though.
OK, let's create CONTRIBUTING.md.
Actually, I used to use the master branch instead of the main branch one year ago, spurred by the rise in racism cases across the US I still want to keep using the master branch. until I start work with you, I remember I saw you are using the main branch and also see others use the main, and now I have get used to the main branch 😆
the rise in racism cases across the US
I don't think political correctness can be enforced in all aspects of life. My radio library projects' examples still use master() for transmitter role and slave() for receiver role. If I were to switch to tx()/rx() it would cause confusion about where the instigating message comes from (because the radio's do full duplex communication)... I also still see "master"/"slave" used in data bus device descriptors...
BTW, there hasn't been a significant rise in racism. You just hear more about it in the news now... Its always been this bad. But at least gay people can still get married (unless the biased supreme court decides to overturn that as well).
I don't how to explain the reason why master change to main branch in english, so I google it and copy it here.
Its always been this bad. But at least gay people can still get married
Peace & cheers~
Trust me. Racism has stayed the same for decades; people who want to hate will find a reason to hate. Don't let Google be that influential. Their "rise in racism" is really more like "rise in reported racism". Of course, this is alarming for progressive white people (like myself), but its not surprising if you've lived in the US for more than a year.
Thank you for the comment, it gave me a little more insight into the topic.
Co-authored-by: Brendan <2bndy5@gmail.com>
Co-authored-by: Brendan <2bndy5@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple things to fix before merging, then I'm ready to merge and submit a solution for #95
No description provided.