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

Change action from docker to composite #83

Merged
merged 28 commits into from
Sep 1, 2022
Merged

Change action from docker to composite #83

merged 28 commits into from
Sep 1, 2022

Conversation

shenxianpeng
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@2bndy5 2bndy5 left a 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!

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
.github/workflows/cpp-linter.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 12, 2022

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).

action.yml Outdated Show resolved Hide resolved
@shenxianpeng shenxianpeng added the enhancement New feature or request label Aug 13, 2022
@shenxianpeng
Copy link
Collaborator Author

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-commenter
Copy link

codecov-commenter commented Aug 19, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@df06936). Click here to learn what that means.
The diff coverage is n/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.

@shenxianpeng
Copy link
Collaborator Author

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.

@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 22, 2022

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.

@shenxianpeng shenxianpeng marked this pull request as draft August 23, 2022 01:31
@shenxianpeng
Copy link
Collaborator Author

I could remove the python src in this PR

Sure, thank you!

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Aug 23, 2022

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.

@2bndy5

This comment was marked as outdated.

@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 24, 2022

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::"

@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 24, 2022

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-fail is false, then the action will return an unsuccessful exit code (1) but only if checks failed.
  • If quiet-fail is 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.

@shenxianpeng
Copy link
Collaborator Author

I think this PR is ready for review.

@shenxianpeng shenxianpeng marked this pull request as ready for review August 31, 2022 09:24
@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 31, 2022

I'm already getting confused which branch is for which version. Can we rename the newer branch v2.x?

@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 31, 2022

I also need to add a warning to the setup.py about using the master main branch during pip install.

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Aug 31, 2022

Sorry, I should clarify what I did about switching branches:

  1. Released the last v1 release, the tag is v1.5.0 from the master branch
  2. Create main branch from master as our future branch
  3. Change the default branch from master to main
  4. 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?

@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 31, 2022

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.

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Sep 1, 2022

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?

@2bndy5
Copy link
Collaborator

2bndy5 commented Sep 1, 2022

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.

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Sep 1, 2022

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 😆

@2bndy5
Copy link
Collaborator

2bndy5 commented Sep 1, 2022

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).

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Sep 1, 2022

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~

image

@2bndy5
Copy link
Collaborator

2bndy5 commented Sep 1, 2022

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.

@shenxianpeng
Copy link
Collaborator Author

Thank you for the comment, it gave me a little more insight into the topic.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
shenxianpeng and others added 2 commits September 1, 2022 15:06
Copy link
Collaborator

@2bndy5 2bndy5 left a 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

.github/workflows/cpp-linter.yml Outdated Show resolved Hide resolved
.github/workflows/cpp-linter.yml Outdated Show resolved Hide resolved
@2bndy5 2bndy5 merged commit bfc328f into main Sep 1, 2022
@2bndy5 2bndy5 deleted the composite-action branch September 1, 2022 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants