Skip to content

Build node bridge in CI#14622

Merged
mroz22 merged 1 commit intodevelopfrom
grdddj/ci_node_bridge_build
Oct 1, 2024
Merged

Build node bridge in CI#14622
mroz22 merged 1 commit intodevelopfrom
grdddj/ci_node_bridge_build

Conversation

@grdddj
Copy link
Copy Markdown
Contributor

@grdddj grdddj commented Sep 30, 2024

Tries to build an exportable artifact with bin.js file containing node-bridge code

To be used by tenv - taking the latest artefact from develop

@grdddj grdddj force-pushed the grdddj/ci_node_bridge_build branch 3 times, most recently from 3a4f216 to 18cc925 Compare September 30, 2024 14:37
@grdddj grdddj requested review from mroz22 and vdovhanych September 30, 2024 14:37
@grdddj grdddj marked this pull request as ready for review September 30, 2024 14:37
@grdddj
Copy link
Copy Markdown
Contributor Author

grdddj commented Sep 30, 2024

Seems to work fine already - see https://github.com/trezor/trezor-suite/actions/runs/11105572536

Consideration - instead (in addition) to saving this as GH-Actions artifacts, it might be beneficial to save it on dev.trezor.io as well ... one benefit is that we would not need ani GH API keys to get it nightly into tenv, and it would be just one non-changing URL

  • there are already some CI jobs in Suite uploading stuff there, I guess we would just need to agree on (and create?) a specific folder (URL) for it
  • in that case, we would change it to run only on develop branch probably, so it is not being rewritten by other branches

Comment thread .github/workflows/build-node-bridge.yml
@mroz22
Copy link
Copy Markdown
Contributor

mroz22 commented Oct 1, 2024

I am not familiar with dev.trezor.io but we are using dev.suite.sldev.cz heavily. For example, recently we added a webusb testere utility here https://dev.suite.sldev.cz/transport-test/develop/.

I am totally ok for using it for this artifact. It should be uploaded probably only for develop branch so that it doesn't bloat too much.

@grdddj grdddj force-pushed the grdddj/ci_node_bridge_build branch 2 times, most recently from 0780506 to 7f208a6 Compare October 1, 2024 07:48
Comment thread .github/workflows/build-node-bridge.yml Outdated
- name: Upload dist to dev server
shell: bash
env:
DEPLOY_PATH: s3://dev.suite.sldev.cz/transport-bridge/dist
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am ok with it but maybe, for consistency purposes - shouldn't it contain branch name here? consider other jobs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have deleted the branch name on purpose, as it will only run in develop branch ... but will include it again


- name: Install deps and build libs
run: |
yarn install --immutable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this full yarn install needed? wouldn't yarn focus to transport-bridge folder suffice?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No idea at all here, but will try it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this way it installs all the dependencies. In frequently run jobs, it makes sense to optimize it using yarn focus. here it is less relevant but still a good practice to follow if possible

@grdddj grdddj force-pushed the grdddj/ci_node_bridge_build branch from 6436b63 to d40acdb Compare October 1, 2024 09:27
@mroz22 mroz22 merged commit eb3d6d4 into develop Oct 1, 2024
@mroz22 mroz22 deleted the grdddj/ci_node_bridge_build branch October 1, 2024 11:43
branches:
- develop
pull_request:
types: [labeled]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes no sense; it will build a node bridge for any PR that you add a label to. Why??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because I do not know workflow syntax well and I copied it like this from some other .yml file. Agreed that it is redundant, I will remove it.

Do you have some other possible improvements, when I will be already making another PR with changes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was the only thing that i noticed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for that. I created a PR for it - #14742

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