-
Notifications
You must be signed in to change notification settings - Fork 21
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
Change action from docker to composite #83
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 I think I can retry using the REST API for |
Note: Instead of retag the current tag still to |
Codecov Report
@@ 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. |
Sure, thank you! |
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 # 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
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 |
I also need to add a warning to the setup.py about using the master |
Sorry, I should clarify what I did about switching branches:
I'd like to merge all the future changes to the main branch and release
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 |
Me too 😃 I still want to use 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 Of course, I'm not against using |
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 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 😆 |
I don't think political correctness can be enforced in all aspects of life. My radio library projects' examples still use 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). |
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 <[email protected]>
Co-authored-by: Brendan <[email protected]>
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.