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(icons): update ci-check to fail with exit code 1 #3856

Merged

Conversation

joshblack
Copy link
Contributor

When handling our refactor, we had assumed console.error would set process.exitCode to 1, instead the ci-check fails silently (with exitCode as 0). This updates it so that the catch handler exits with an exit code of 1, signaling that an error has occurred and failing CI.

Changelog

New

Changed

  • Add process.exit(1) to fail with code 1

Removed

@joshblack joshblack requested a review from a team August 28, 2019 15:34
@project-bot project-bot bot added this to PR: needs review 🕵️ in Carbon support Aug 28, 2019
@ghost ghost requested review from asudoh and tw15egan and removed request for a team August 28, 2019 15:34
@netlify
Copy link

netlify bot commented Aug 28, 2019

Deploy preview for the-carbon-components ready!

Built with commit 16818aa

https://deploy-preview-3856--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Aug 28, 2019

Deploy preview for carbon-elements ready!

Built with commit 16818aa

https://deploy-preview-3856--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Aug 28, 2019

Deploy preview for carbon-components-react ready!

Built with commit 16818aa

https://deploy-preview-3856--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Aug 28, 2019

Deploy preview for the-carbon-components ready!

Built with commit facfbe0

https://deploy-preview-3856--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Aug 28, 2019

Deploy preview for carbon-components-react ready!

Built with commit facfbe0

https://deploy-preview-3856--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Aug 28, 2019

Deploy preview for carbon-elements ready!

Built with commit facfbe0

https://deploy-preview-3856--carbon-elements.netlify.com

@vpicone
Copy link
Contributor

vpicone commented Aug 28, 2019

@joshblack is 2d7d006 to test that the fail working properly?

@joshblack
Copy link
Contributor Author

joshblack commented Aug 28, 2019

@vpicone I think we renamed it to 3D-iCa right? It seems like our ci-check didn't pick up that we wanted to remove the asset, if we still want it totally fair! I can add back in the metadata

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

👍

@vpicone
Copy link
Contributor

vpicone commented Aug 28, 2019

@joshblack ah nvm Coulda sworn I renamed it properly in my PR...

Carbon support automation moved this from PR: needs review 🕵️ to PR: ready to merge 🔜 Aug 28, 2019
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me. I guess there will be a follow up PR to add that missing subcategory?

@joshblack
Copy link
Contributor Author

Note to reviewers, couple of TODOs left on my end:

  • Update ci-check to respect no subcategory on pictograms metadata
  • Verify pictograms metadata passes CI checks 👍

@joshblack
Copy link
Contributor Author

Issue to update mis-categorized or misnamed icons: #3877

@joshblack joshblack merged commit a7d7d34 into carbon-design-system:master Aug 30, 2019
Carbon support automation moved this from PR: ready to merge 🔜 to Issue/PR: Closed 🙌 Aug 30, 2019
@joshblack joshblack deleted the fix/update-icon-ci-check branch August 30, 2019 21:13
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.

None yet

5 participants