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

fix(ci): whitenoise lint failed when pull request #1904

Merged
merged 3 commits into from
Dec 21, 2023
Merged

fix(ci): whitenoise lint failed when pull request #1904

merged 3 commits into from
Dec 21, 2023

Conversation

zaunist
Copy link
Contributor

@zaunist zaunist commented Sep 21, 2023

What type of PR is this?

Fix whitenoise lint error

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes 523

@zaunist zaunist marked this pull request as ready for review September 21, 2023 09:19
@zaunist zaunist requested a review from a team as a code owner September 21, 2023 09:19
@zaunist zaunist changed the title ci: add GITHUB default env fix(ci): whitenoise lint faile when pull request Sep 21, 2023
@zaunist zaunist changed the title fix(ci): whitenoise lint faile when pull request fix(ci): whitenoise lint failed when pull request Sep 21, 2023
@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c2f88f0) 64.59% compared to head (180188c) 64.35%.

❗ Current head 180188c differs from pull request most recent head 98dc5d7. Consider uploading reports for the commit 98dc5d7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1904      +/-   ##
==========================================
- Coverage   64.59%   64.35%   -0.24%     
==========================================
  Files         112      109       -3     
  Lines       16072    15302     -770     
==========================================
- Hits        10382     9848     -534     
+ Misses       5038     4845     -193     
+ Partials      652      609      -43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Xunzhuo
Copy link
Member

Xunzhuo commented Sep 21, 2023

image

Thanks for working on it, I did not look deep into this issue, but it seems like still throw an error in Pull Request.

@zaunist
Copy link
Contributor Author

zaunist commented Sep 21, 2023

image Thanks for working on it, I did not look deep into this issue, but it seems like still throw an error in Pull Request.

You're right, I'm not entirely sure about the exact problem either. This is my first test, and I will further study how to solve this issue carefully.

@zaunist
Copy link
Contributor Author

zaunist commented Sep 21, 2023

Hi, @Xunzhuo. I've identified the issue. In the github pull request CI environment, there is no branch information available, causing the git diff command to fail. Therefore, I added a git fetch command to retrieve the remote branch information and prefixed the branch names with "origin" to match them.

Here is my debug info for reference.

image

https://github.com/zaunist/gateway/actions/runs/6262614026/job/17005194717

@Xunzhuo
Copy link
Member

Xunzhuo commented Sep 22, 2023

/retest

Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Nov 10, 2023
@arkodg
Copy link
Contributor

arkodg commented Nov 10, 2023

@zaunist DCO is failing, can you squash, sign and push again ?

@arkodg arkodg removed the stale label Nov 10, 2023
@zaunist
Copy link
Contributor Author

zaunist commented Nov 14, 2023

@zaunist DCO is failing, can you squash, sign and push again ?

Done

@arkodg
Copy link
Contributor

arkodg commented Nov 14, 2023

DCO is still failing, you might need to squash commits and then resign and push

@@ -1,7 +1,8 @@
#!/bin/bash
TRAILING_WHITESPACE=0
# only check changed docs in pr
for file in $(git diff --cached --name-only --diff-filter=ACRMTU $GITHUB_BASE_REF | grep "\.md"); do
git fetch
for file in $(git diff --cached --name-only --diff-filter=ACRMTU origin/$GITHUB_BASE_REF| grep "\.md"); do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking out loud, why isn't git diff --name-only good enough here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that the GitHub Actions environment does not have the branch information for Git, which causes an error when using git diff to compare the differences. To resolve this, I simply added a git fetch command to retrieve the branch information from the remote repository. For further debugging information, you can refer to this link: https://github.com/zaunist/gateway/actions/runs/6262614026/job/17005194717.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zaunist im suggesting removing the older command and replacing with something simpler git diff --name-only which doesnt require branch info, do you see any issue with this approach ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arkodg If we don't provide branch information, how can we compare the differences between the fork and main branches? I'm not quite understanding this point. Could you provide an example, please?

Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am OK with current approach, thanks for working on it @zaunist.

Actually I think this one is not that high priority, if you would like to help, we can try to pick some important issues for you : )

@Xunzhuo Xunzhuo merged commit 844157a into envoyproxy:main Dec 21, 2023
9 checks passed
@zaunist
Copy link
Contributor Author

zaunist commented Jan 2, 2024

I am OK with current approach, thanks for working on it @zaunist.

Actually I think this one is not that high priority, if you would like to help, we can try to pick some important issues for you : )

Willing to make more contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whitenoise linter is not working well in GA
4 participants