Skip to content

Use separate jobs instead of child_process #881

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

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

Conversation

amvanbaren
Copy link
Contributor

This PR introduces the use of separate jobs instead of child_process to sandbox the extension build process.

@amvanbaren amvanbaren force-pushed the build-extension branch 3 times, most recently from 3a405c3 to 66976d2 Compare May 21, 2025 09:54
Copy link
Contributor

@tfroment tfroment left a comment

Choose a reason for hiding this comment

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

+1

@tfroment
Copy link
Contributor

tfroment commented Jun 2, 2025

@amvanbaren
Copy link
Contributor Author

The reporting functionality is broken:

# TODO fix reporting
# - name: Report results
# run: bun run ./report-extensions.ts
# - uses: actions/upload-artifact@v4
# if: always()
# with:
# name: report
# path: |
# /tmp/stat.json
# /tmp/result.md
# - uses: actions/upload-artifact@v4
# if: always()
# with:
# name: artifacts
# path: |
# /tmp/artifacts/*.vsix
# - name: Upload job summary
# if: always()
# run: cat /tmp/result.md >> $GITHUB_STEP_SUMMARY
# - name: Get previous job's status
# id: lastrun
# uses: filiptronicek/get-last-job-status@main
# - name: Slack Notification
# if: ${{ !github.event.inputs.extensions && ((success() && steps.lastrun.outputs.status == 'failed') || failure()) }}
# uses: rtCamp/action-slack-notify@v2
# env:
# SLACK_WEBHOOK: ${{ secrets.GITPOD_SLACK_WEBHOOK }}
# SLACK_COLOR: ${{ job.status }}

There's a check for important extensions: https://github.com/EclipseFdn/publish-extensions/blob/f7cfb5c517bbf53d772704383db8b22b4cb895aa/report-extensions.ts#L226C1-L232C2

Because the reporting functionality is broken every extension is seen as outdated and the Validate PR job fails.

Copy link
Member

@filiptronicek filiptronicek left a comment

Choose a reason for hiding this comment

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

In general, I'm aligned and happy with this approach --> we never really did publishing outside of GitHub Actions anyway.

I'm curious about:

  • How does our local testing story look like? I used to run node publish-extension.js with a list of extensions when debugging build issues with certain extensions
  • How can we test the PR before landing it? If it's not worth the effort to do here, I'm also fine with merging it and testing then
  • Do we have an idea on how this will affect timing? I know there's some concurrency limits for GHA workflows, so given there's > 400 extensions, I worry the publishing times might get lengthy.

const publishContext = JSON.parse(process.env.PUBLISH_CONTEXT);
publishContext.msLastUpdated = new Date(publishContext.msLastUpdated);
publishContext.ovsxLastUpdated = new Date(publishContext.ovsxLastUpdated);
await resolveExtension(
Copy link
Member

Choose a reason for hiding this comment

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

Does resolveExtension have a side effect we need here or can we safely remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like finally not having everything in the root 😄

Copy link
Member

Choose a reason for hiding this comment

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

Can you please run npm run format here?

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