Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
2bndy5 merged 28 commits into main from composite-action
Sep 1, 2022
Merged

Change action from docker to composite #83

2bndy5 merged 28 commits into main from composite-action
Sep 1, 2022

Conversation

@shenxianpeng
Copy link
Collaborator

@shenxianpeng shenxianpeng commented Aug 11, 2022

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!

Copy link
Collaborator

2bndy5 commented Aug 12, 2022
edited
Loading

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

@shenxianpeng shenxianpeng added the enhancement New feature or request label Aug 13, 2022
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

2bndy5 reacted with thumbs up emoji

Copy link

codecov-commenter commented Aug 19, 2022
edited
Loading

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.

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.

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
Copy link
Collaborator Author

I could remove the python src in this PR

Sure, thank you!

Copy link
Collaborator Author

shenxianpeng commented Aug 23, 2022
edited
Loading

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.

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

Copy link
Collaborator

2bndy5 commented Aug 24, 2022
edited
Loading

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 shenxianpeng changed the base branch from master to main August 28, 2022 05:27
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
Copy link
Collaborator

2bndy5 commented Aug 31, 2022
edited
Loading

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

Copy link
Collaborator

2bndy5 commented Aug 31, 2022
edited
Loading

I also need to add a warning to the setup.py about using the master (削除) main (削除ここまで) branch during pip install.

shenxianpeng reacted with thumbs up emoji

Copy link
Collaborator Author

shenxianpeng commented Aug 31, 2022
edited
Loading

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?

Copy link
Collaborator

2bndy5 commented Aug 31, 2022
edited
Loading

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.

Copy link
Collaborator Author

shenxianpeng commented Sep 1, 2022
edited
Loading

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?

Copy link
Collaborator

2bndy5 commented Sep 1, 2022
edited
Loading

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.

Copy link
Collaborator Author

shenxianpeng commented Sep 1, 2022
edited
Loading

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 reacted with laugh emoji

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

Copy link
Collaborator Author

shenxianpeng commented Sep 1, 2022
edited
Loading

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

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.

Copy link
Collaborator Author

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

shenxianpeng and others added 2 commits September 1, 2022 15:06
Co-authored-by: Brendan <2bndy5@gmail.com>
Co-authored-by: Brendan <2bndy5@gmail.com>
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

shenxianpeng reacted with thumbs up emoji shenxianpeng reacted with hooray emoji
@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

Reviewers

@2bndy5 2bndy5 2bndy5 approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

No open projects

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /