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

CI: output fixes #102

Merged
merged 2 commits into from
Jan 3, 2024
Merged

CI: output fixes #102

merged 2 commits into from
Jan 3, 2024

Conversation

fredericlepied
Copy link
Contributor

@fredericlepied fredericlepied commented Dec 19, 2023

  • sanity: capture the output for all the versions of Python for the summary
  • ansible-lint: display the output of ansible-lint on the PR in the job detail view
  • ansible-lint: use colors like in Sanity Check
  • check-all-dependencies-are-merged: do not checkout the code

Copy link

@fredericlepied fredericlepied force-pushed the fix-output branch 2 times, most recently from ce4cf41 to d0df895 Compare December 19, 2023 22:06
Copy link

Copy link

@fredericlepied fredericlepied requested a review from a team as a code owner December 19, 2023 23:22
@dcibot
Copy link
Collaborator

dcibot commented Dec 19, 2023

Starting dci-check-change job.

Copy link

@thekad
Copy link
Contributor

thekad commented Dec 19, 2023

  1. your PR title says fix the sanity check, but you're changing the ansible-lint check
  2. the output is exactly the same as the previous output, but you're making it harder by forcing color output then striping the colors out of it, what exactly are you trying to accomplish here?

@tonyskapunk
Copy link
Contributor

I can answer about this one 😸

2. the output is exactly the same as the previous output, but you're making it harder by forcing color output then striping the colors out of it, what exactly are you trying to accomplish here?

The output is different as it includes coloring in the step when ansible-lint is running, here: https://github.com/redhatci/ansible-collection-redhatci-ocp/actions/runs/7268748379/job/19805160605?pr=102#step:6:162 vs https://github.com/redhatci/ansible-collection-redhatci-ocp/actions/runs/7267875512/job/19802603666#step:6:44

The end result is, 1. Visually better output in the detailed step run and 2. Keep the markdown results clear (striping of ansi codes)

hth

@thekad
Copy link
Contributor

thekad commented Dec 19, 2023

I can answer about this one 😸

2. the output is exactly the same as the previous output, but you're making it harder by forcing color output then striping the colors out of it, what exactly are you trying to accomplish here?

The output is different as it includes coloring in the step when ansible-lint is running, here: https://github.com/redhatci/ansible-collection-redhatci-ocp/actions/runs/7268748379/job/19805160605?pr=102#step:6:162 vs https://github.com/redhatci/ansible-collection-redhatci-ocp/actions/runs/7267875512/job/19802603666#step:6:44

The end result is, 1. Visually better output in the detailed step run and 2. Keep the markdown results clear (striping of ansi codes)

hth

I see, you want the colors to be displayed since it's changing a > for a | tee and they would be pretty printed in the detail view. I still say that accomplishes the same thing at the end when building the summary because you strip the coloring codes anyway, but I get it.

@tonyskapunk
Copy link
Contributor

I'm removing the test commit added, since it was just for demo/test purpose 😺 here the link to the summary: https://github.com/redhatci/ansible-collection-redhatci-ocp/actions/runs/7268748379#summary-19805160605

@tonyskapunk
Copy link
Contributor

I see, you want the colors to be displayed since it's changing a > for a | tee and they would be pretty printed in the detail view.

I'm assuming the same 😄 I'm reviewing the change and providing a fix to make sure the ansi code does not get into the markdown for the summary.

I still say that accomplishes the same thing at the end when building the summary because you strip the coloring codes anyway, but I get it.

I think the main difference is that the coloring in the step details helps for reading, the summary is well, a summary only :)

Copy link

tonyskapunk
tonyskapunk previously approved these changes Dec 19, 2023
Copy link
Contributor

@tonyskapunk tonyskapunk left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd like someone else to review due to the changes I've introduced 😅

@dcibot
Copy link
Collaborator

dcibot commented Dec 20, 2023

@fredericlepied
Copy link
Contributor Author

  1. your PR title says fix the sanity check, but you're changing the ansible-lint check

Agreed. Fixing the commit the message. It was too late yesterday ;-)

2. the output is exactly the same as the previous output, but you're making it harder by forcing color output then striping the colors out of it, what exactly are you trying to accomplish here?

It is not the same as we do see the actual reports in the detail view if someone wants to fix them else we don't see anything. I did it like this to be consistent the Sanity test job.

Copy link

@fredericlepied fredericlepied changed the title fix Sanity test output capture CI: output fixes Dec 20, 2023
.github/workflows/pr.yml Outdated Show resolved Hide resolved
.github/workflows/pr.yml Outdated Show resolved Hide resolved
.github/workflows/pr.yml Outdated Show resolved Hide resolved
Copy link

Copy link

Copy link

Copy link

Copy link

@thekad
Copy link
Contributor

thekad commented Jan 2, 2024

@fredericlepied can you rebase? that will trigger a rebuild anyway, seems like it was a temporary error

Copy link

Copy link

Copy link

fredericlepied and others added 2 commits January 3, 2024 18:32
- sanity: capture the output for all the versions of Python for the summary
- ansible-lint: display the output of ansible-lint on the PR in the job detail view
- ansible-lint: use colors like in Sanity Check
- check-all-dependencies-are-merged: do not checkout the code
Removes the ansi code before adding it to the GH step summary for a
propoer reading
Copy link

@fredericlepied fredericlepied merged commit 8172b85 into main Jan 3, 2024
5 checks passed
@fredericlepied fredericlepied deleted the fix-output branch January 3, 2024 17:51
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.

4 participants