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 results comment #2068

Merged
merged 3 commits into from
Mar 28, 2024
Merged

fix ci results comment #2068

merged 3 commits into from
Mar 28, 2024

Conversation

moukoublen
Copy link
Member

@moukoublen moukoublen commented Mar 28, 2024

Summary of your changes

Fix the missing comment in PRs with allure report.

Screenshot/Data

Related Issues

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary README/documentation (if appropriate)

Introducing a new rule?

Copy link

mergify bot commented Mar 28, 2024

This pull request does not have a backport label. Could you fix it @moukoublen? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
    NOTE: backport-skip has been added to this pull request.

@elastic elastic deleted a comment from github-actions bot Mar 28, 2024
@elastic elastic deleted a comment from github-actions bot Mar 28, 2024
Copy link

❌ No tests were run ❌

Copy link

github-actions bot commented Mar 28, 2024

📊 Allure Report - 💚 No failures were reported.

Result Count
🟥 Failed 0
🟩 Passed 359
⬜ Skipped 33

@moukoublen moukoublen force-pushed the fix_ci_summary branch 3 times, most recently from 910a4d9 to 97c0bda Compare March 28, 2024 11:38
@moukoublen moukoublen marked this pull request as ready for review March 28, 2024 11:55
@moukoublen moukoublen requested a review from a team as a code owner March 28, 2024 11:55
ls -lahR || true
rm -f tests/allure/results/cover.out || true
ls -lahR tests/allure/results/ || true
echo "event_name: ${{ github.event_name }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@moukoublen, to prevent script injection, it's better to store github.event_name as an environment variable.

shell: bash
env:
    EVENT_NAME: ${{ github.event_name }}
run: |
    ls -lahR tests/allure/results/ || true
    echo "event_name: ${EVENT_NAME}"

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to delete that entirely; I just put it there for debugging reasons while trying things. I will delete it.

(Also I think in this specific case, the event_name is predefined by github and not related to user input)

@@ -298,6 +298,7 @@ jobs:
timeout-minutes: 60
permissions:
pull-requests: write
checks: write
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this permission?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I am not sure if we actually need it. We shouldn't.

The issue is that summary entries and comments do not appear in forks-originated PRs; they do appear in PRs from this repo.

So my initial assumption is that:

  • Either github.event_name in case of pull_request_target is not 'pull_request' (so I changed the if to if: ${{ success() && github.event_name != 'push' }})
  • Or this permission is needed (because it is also present in this workflow )

Do you think we should first merge the first only, and if it does not work, then we should try the check: write permission?

Copy link
Collaborator

@gurevichdmitry gurevichdmitry Mar 28, 2024

Choose a reason for hiding this comment

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

Let's attempt to merge without it (checks: write), and if necessary, we can add this permission later.

@moukoublen moukoublen merged commit a3e2c32 into main Mar 28, 2024
35 checks passed
@moukoublen moukoublen deleted the fix_ci_summary branch March 28, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants