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.

Sign up for GitHub

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

Jump to bottom

build: log npm run postinstall steps to STDOUT so they are visible in CI #33142

Merged
merged 3 commits into from
Oct 4, 2023
Merged

build: log npm run postinstall steps to STDOUT so they are visible in CI #33142

merged 3 commits into from
Oct 4, 2023

Conversation

kdmccormick
Copy link
Member

Without this change, npm run postinstall (aka scripts/copy-node-modules.sh) uses set -x, which echoes steps to STDERR. By default, it seems that GitHub Actions doesn't show STDERR.

Having the steps visible in CI was very useful while debugging some Tutor build improvements, so I figured it would be good to upstream the change.

@kdmccormick
build: log npm run postinstall steps to STDOUT so they are visible …
de10d97 
…in CI

Without this change, `npm run postinstall` (aka
scripts/copy-node-modules.sh) uses `set -x`, which echo steps to STDERR.
By default, it seems that GitHub Actions doesn't show STDERR.

Having the steps visible in CI was very useful while debugging some
Tutor build improvements, so I figured it would be good to upstream the
change.
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 31, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @kdmccormick! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

⚠️ We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by editing the PR title. If the problem persists, you can tag the @openedx/cla-problems team in a comment on your PR for further assistance.

@kdmccormick kdmccormick mentioned this pull request Aug 31, 2023
Closed
@kdmccormick kdmccormick requested a review from feanil August 31, 2023 12:41
@nedbat
Copy link
Contributor

nedbat commented Aug 31, 2023

Bot, please re-consider.

feanil
feanil approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

This seems fine, but is there a reason to do this over just piping stderr to stdout in CI if the issue is a shortcoming of CI? eg. npm run postinstall 2>&1

@kdmccormick kdmccormick removed the open-source-contribution PR author is not from Axim or 2U label Aug 31, 2023
@kdmccormick kdmccormick enabled auto-merge (squash) October 4, 2023 17:13
@kdmccormick kdmccormick merged commit c116371 into openedx:master Oct 4, 2023
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@kdmccormick kdmccormick deleted the kdmccormick/npm-ci-echo branch October 5, 2023 20:38
Ali-Salman29 pushed a commit to edly-io/edx-platform that referenced this pull request Dec 4, 2023
@kdmccormick
build: log npm run postinstall steps to STDOUT so they are visible …
6555adc 
…in CI (openedx#33142)

Without this change, `npm run postinstall` (aka
scripts/copy-node-modules.sh) uses `set -x`, which echo steps to STDERR.
By default, it seems that GitHub Actions doesn't show STDERR.

Having the steps visible in CI was very useful while debugging some
Tutor build improvements, so I figured it would be good to upstream the
change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feanil feanil feanil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kdmccormick @openedx-webhooks @nedbat @edx-pipeline-bot @feanil @e0d