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

Colorize git #1726

Merged
merged 17 commits into from
Mar 24, 2021
Merged

Colorize git #1726

merged 17 commits into from
Mar 24, 2021

Conversation

FredericoGauz
Copy link
Contributor

Details

Adds a better way to see differences between action diffs.

Fixed Issues

#1665

Changes

Adds a temporary Library to the GitHub workflow that reflect better the changes in files

@FredericoGauz FredericoGauz requested a review from a team as a code owner March 11, 2021 14:51
@botify botify requested review from bondydaa and removed request for a team March 11, 2021 14:52
@roryabraham
Copy link
Contributor

Also @FredericoGauz, when you update this PR could you please include screenshots as well?

@FredericoGauz
Copy link
Contributor Author

Sure!

git diff --exit-code | diff-so-fancy | less --tabs=4 -RFX

# Runs git diff quietly to get the exit code
declare EXIT_CODE
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm does the exit-code from the first git diff basically get swallowed up by the other piped commands? If so, I would just get rid of the --exit-code flag in the first git diff run. I'd be willing to bet that there's a clean way to propagate that exit code through the piped diff-so-fancy and less commands though, which would be preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @roryabraham , I was going to reply you and David after finishing here.
I totally agree with you, but after running many tests with many options (like trying to get from this line: git diff --exit-code | diff-so-fancy | less --tabs=4 -RFX) and also (if [[ EXIT_CODE -eq 0 ]]; ) this directly nothing gave the the first exist code of the pipeline. I tried for a few hours with all the things I could think of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I understand that an extra variable is not ideal, that was the best I could do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I suck at using bash so I'm not sure I have a better suggestion. I'm not super worried about it, but maybe @bondydaa knows how to do this with some bash wizardry?

@FredericoGauz FredericoGauz requested a review from bondydaa March 20, 2021 16:34
@roryabraham
Copy link
Contributor

roryabraham commented Mar 22, 2021

@FredericoGauz Can you please retest this by:

  1. Make some changes to the Github Actions scripts
  2. commit and push those changes
  3. Provide screenshots and a link to the VerifyGithubActionsBuilds workflows that show the character-level diff
  4. Revert the changes, commit and push.

Copy link
Contributor

@bondydaa bondydaa left a comment

Choose a reason for hiding this comment

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

I have this working locally like so
image

image

image

@FredericoGauz
Copy link
Contributor Author

Hi @roryabraham and @bondydaa, sorry for the late reply. I did the changes requested (it was quite clever to save the whole output in a variable!) and also run the tests you required.

Screenshot 2021-03-24 06 03 33 (1)

The link for the failing action is: https://github.com/Expensify/Expensify.cash/pull/1726/checks?check_run_id=2184601524

I hope that this is what is need, but let me know if I can help with anything else!

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Okay, LGTM. All yours @bondydaa

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