-
Notifications
You must be signed in to change notification settings - Fork 32
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
add workflow to build all Rust apps for every PR #125
Conversation
335545d
to
6e2ce74
Compare
dd0e3d4
to
4b3cdf0
Compare
.github/workflows/build_all_apps.yml
Outdated
matrix: | ||
include: | ||
- repo: 'app-mobilecoin' | ||
branch: 'develop' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the default branch instead? Though this might be less clear...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Shall wait develop
branch to be merged into main/master
in that case.
c3a817b
to
3224ad1
Compare
.github/workflows/build_all_apps.yml
Outdated
include: | ||
#- repo: 'app-mobilecoin' | ||
# branch: 'develop' | ||
- repo: 'app-radix-babylon' | ||
branch: 'develop' | ||
- repo: 'app-boilerplate-rust' | ||
branch: 'main' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be good to not have an hardcoded list but to build dynamically the list.
I tried to do that in this PR for C app (name: Get C apps repos)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we need a way to retrieve all apps in multiples places:
- Rust SDK CI
- C SDK CI
- app tester?
Maybe it could be moved to ledgered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it could be a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I will update the workflow accordingly when the feature will be provided by ledgered
. in the meantime, we should proceed with this manual listing as it fulfils the requirement (build all Rust apps with the PR-modified Rust SDK to check if any regression).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no one adds it to ledgered it's never going to be there ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lucas will do when he will have the bandwidth
43c7769
to
102a7a0
Compare
No description provided.