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

add workflow to build all Rust apps for every PR #125

Merged
merged 6 commits into from
Apr 8, 2024
Merged

Conversation

yogh333
Copy link
Contributor

@yogh333 yogh333 commented Jan 25, 2024

No description provided.

@yogh333 yogh333 force-pushed the jca/build_all_apps branch 2 times, most recently from dd0e3d4 to 4b3cdf0 Compare February 6, 2024 08:48
@yogh333 yogh333 marked this pull request as ready for review February 6, 2024 08:54
.github/workflows/build_all_apps.yml Outdated Show resolved Hide resolved
matrix:
include:
- repo: 'app-mobilecoin'
branch: 'develop'
Copy link
Contributor

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...

Copy link
Contributor Author

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.

.github/workflows/build_all_apps.yml Outdated Show resolved Hide resolved
Comment on lines 19 to 30
include:
#- repo: 'app-mobilecoin'
# branch: 'develop'
- repo: 'app-radix-babylon'
branch: 'develop'
- repo: 'app-boilerplate-rust'
branch: 'main'

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)

Copy link
Contributor

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?

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

Copy link
Contributor Author

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).

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

@xchapron-ledger xchapron-ledger Apr 8, 2024

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 ;)

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

@yogh333 yogh333 merged commit a882f1d into master Apr 8, 2024
30 checks passed
@yogh333 yogh333 deleted the jca/build_all_apps branch April 8, 2024 16:50
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