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

Optimize the diff text when there's no change #3036

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented May 18, 2024

If a PR doens't generate a diff (like this change) then the message shouldn't link to any diffs. By making this message explicit it makes reviewing a bit easier.

Please cherry-pick my commits into:

  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • Foreman 3.4/Katello 4.6 (EL8 only)
  • Foreman 3.3/Katello 4.5 on EL7 & EL8 (Satellite 6.12 on EL8 only; orcharhino 6.4/6.5 on EL8 only)
  • We do not accept PRs for Foreman older than 3.3.

Copy link

github-actions bot commented May 18, 2024

The PR preview for 3b74f72 is available at theforeman-foreman-documentation-preview-pr-3036.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@ekohl
Copy link
Member Author

ekohl commented May 18, 2024

The following output files are affected by this PR:

show diff

show diff as HTML

This suggests it's not working as intended, but the workflow runs after deploy finished and uses the defintiion from core rather than the PR.

Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

Lovely idea. Not sure about the implementation, though :)

.github/workflows/preview.yml Outdated Show resolved Hide resolved
.github/workflows/preview.yml Outdated Show resolved Hide resolved
.github/workflows/preview.yml Show resolved Hide resolved
@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Not yet reviewed labels May 20, 2024
@ekohl
Copy link
Member Author

ekohl commented May 20, 2024

When testing the implementation appeared to have an issue, so I need to dive into it a bit more. Though perhaps that was in the other repo I was playing with

@maximiliankolb
Copy link
Contributor

triage: @ekohl friendly reminder

@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Jan 17, 2025
If a PR doens't generate a diff (like this change) then the message
shouldn't link to any diffs. By making this message explicit it makes
reviewing a bit easier.
@ekohl
Copy link
Member Author

ekohl commented Jan 17, 2025

I've fixed the theni and also modified some paths, but as we've just seen: this thing is hard to test.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -68,8 +68,10 @@ jobs:
- name: Create diff to old base
run: |
diff -Nrwu old/ preview/${{ fromJSON(env.PR_DATA).target_name }}/ | cat > preview/diff.patch
pygmentize -o preview/diff.html -l diff -f html -O full preview/diff.patch
diffstat -l -p2 preview/diff.patch > diff.txt
if [[ -s preview/diff.patch ]] ; then
Copy link
Member Author

Choose a reason for hiding this comment

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

Potential performance improvement: only install diffstat python3-pygments if there is a diff.

@ekohl ekohl requested a review from evgeni January 20, 2025 20:33
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.

3 participants