-
Notifications
You must be signed in to change notification settings - Fork 24
Fix test CI issue by calling self test action #217
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
latest tag was added to the incorrect commit hash (追記ここまで)
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
We have push events disabled so we can't test merge commits, which is a bit of a concern for me.
but if we enable the push event, we will see the following comments in both the merge commit and the pull request here.
if we set thread-comments: false, we will miss a feature test. That is a problem.
Cpp-Linter Report
⚠️ Some files did not pass the configured checks!
clang-format reports: 2 file(s) not formatted
clang-tidy reports: 6 concern(s)
Have any feedback or feature suggestions? Share it here.
We are seeing the thread-comment in this thread because the GITHUB_REPOSITORY env var is set to "cpp-linter/cpp-linter-action" (not "cpp-linter/test-cpp-linter-action"). There are other problems that we have been trying to workaround as well, all the problems relate to the fact: The cpp-linter package is not meant to be executed on a certain repo that is not same repo triggering the workflow.
The cpp-linter package is not meant to be executed on a certain repo that is not same repo triggering the workflow.
I don't think this is a feature anyone would want in cpp-linter because it seems specific to our use case.
I think the solution is to switch back to running cpp-linter on this repo. Instead of using docs/examples/demo folder, we could add the test repo as a submodule of this repo and set ignore to explicitly not ignore the submodule. With the submodule checked out, we could then call a reusable workflow from the submodule's path (I think).
With the submodule checked out, we could then call a reusable workflow from the submodule's path (I think).
This idea won't work exactly as stated. We can't checkout a submodule (in 1 job) then call the reusable workflow from the submodule's path (in another job).
Remember, the changes to this branch are not tested yet. The pull_request_target executes in the context of the target (aka base) branch, so all changes in this head branch are basically ignored.
f5b0423 to
aaa1ce7
Compare
The cpp-linter package is not meant to be executed on a certain repo that is not same repo triggering the workflow.
if possible to change GITHUB_REPOSITORY to cpp-linter/test-cpp-linter-action before running test action?
According to GH docs:
You can't overwrite the value of the default environment variables named
GITHUB_*andRUNNER_*.
Docs about using on.push.tags
This seems more applicable to the triger that calls the reusable workflow:
on: push: tags: - latest
It seems we don't need to call reusable workflow from cpp-linter/.github, we can use uses: ./ to self test action like this example
https://github.com/pre-commit/action/blob/main/.github/workflows/main.yml#L12-L13
with that, we may not need to create latest tag for testing
oh, I never thought to use ./. That's very clever.
The self test CI failed because
-
It found errors in the C source files that installed with pygit2. Basically it didn't ignore the venv that it was running from.
-
Body is too long (maximum is 65536 characters).
We really need to review/test conditionally create comment cpp-linter#91 to avoid this "comment too long" error
latest tag was added to the incorrect commit hash (削除ここまで)I ignored the venv path and the CI passed. How about now, ready to merge?
Cpp-Linter Report ⚠️
Some files did not pass the configured checks!
clang-format reports: 2 file(s) not formatted
- docs/examples/demo/demo.cpp
- docs/examples/demo/demo.hpp
clang-tidy reports: 7 concern(s)
-
docs/examples/demo/demo.cpp:3:10: warning: [modernize-deprecated-headers]
inclusion of deprecated C++ header 'stdio.h'; consider using 'cstdio' instead
#include <stdio.h> ^~~~~~~~~ <cstdio>
-
docs/examples/demo/demo.cpp:8:5: warning: [modernize-use-trailing-return-type]
use a trailing return type for this function
int main(){ ~~~ ^ auto -> int
-
docs/examples/demo/demo.cpp:10:13: warning: [readability-braces-around-statements]
statement should be inside braces
for (;;) break; ^ {
-
docs/examples/demo/demo.cpp:13:5: warning: [cppcoreguidelines-pro-type-vararg]
do not call c-style vararg functions
printf("Hello world!\n"); ^
-
docs/examples/demo/demo.hpp:6:11: warning: [modernize-use-default-member-init]
use default member initializer for 'useless'
char* useless; ^ {"0円"}
-
docs/examples/demo/demo.hpp:7:9: warning: [modernize-use-default-member-init]
use default member initializer for 'numb'
int numb; ^ {0}
-
docs/examples/demo/demo.hpp:11:11: warning: [modernize-use-trailing-return-type]
use a trailing return type for this function
void *not_useful(char *str){useless = str;} ~~~~~~^ auto -> void *
Have any feedback or feature suggestions? Share it here.
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.
I understand that you just copied most of the workflow from our test repo, But that workflow is a little neglected.
Co-authored-by: Brendan <2bndy5@gmail.com>
Co-authored-by: Brendan <2bndy5@gmail.com>
Co-authored-by: Brendan <2bndy5@gmail.com>
Co-authored-by: Brendan <2bndy5@gmail.com>
closes #212