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

Sprint 53 #1205

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Sprint 53 #1205

wants to merge 9 commits into from

Conversation

MiguelLZPF
Copy link
Contributor

@MiguelLZPF MiguelLZPF commented Oct 11, 2024

Description:

  • Hashconnect removed from the repository = Hedera Wallet connect must be used for Hashpack
  • Blade removed from the UI/SDK = Hedera Wallet connect replaces it
  • Github actions refactored :
    - Components tests only run if changes apply to the component itself
    - Alpine v3.20 lightweight container set replaces the full ubuntu container. Size reduced from 1.1GB to 140MB
    - Publish workflow subdivided in jobs one per module) so that if one fails the other do not need to be published again

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Miguel_LZPF <[email protected]>
@MiguelLZPF MiguelLZPF marked this pull request as ready for review October 11, 2024 12:02
@MiguelLZPF MiguelLZPF requested review from a team as code owners October 11, 2024 12:02
@rbarkerSL
Copy link
Contributor

@MiguelLZPF please document the Description and Related Issue(s) section of the PR.

@AlbertoMolinaIoBuilders
Copy link
Contributor

@rbarkerSL I filled the description section as suggested

.github/actions/initial-steps/action.yaml Outdated Show resolved Hide resolved
.github/workflows/publish-all.backup.yaml Show resolved Hide resolved
.github/workflows/publish-all.backup.yaml Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/test-contracts.yaml Outdated Show resolved Hide resolved
.github/workflows/test-sdk.yaml Outdated Show resolved Hide resolved
.github/workflows/test-web.yaml Outdated Show resolved Hide resolved
.github/workflows/version.yaml Outdated Show resolved Hide resolved
.husky/pre-commit Show resolved Hide resolved
.github/workflows/publish-all.backup.yaml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/test-backend.yaml Show resolved Hide resolved
.github/workflows/test-web.yaml Show resolved Hide resolved
.github/workflows/version.yaml Show resolved Hide resolved
.github/workflows/version.yaml Show resolved Hide resolved
.github/workflows/version.yaml Outdated Show resolved Hide resolved
.github/workflows/version.yaml Outdated Show resolved Hide resolved
@MiguelLZPF
Copy link
Contributor Author

Hello @mishomihov00 and @rbarkerSL ! Can you review the comments? All were addressed yesterday.

@mishomihov00
Copy link
Contributor

@MiguelLZPF as described in this comment - https://github.com/hashgraph/stablecoin-studio/pull/1205/files#r1808173072, please either add the "Hardened runner" step as the first step in the workflows or invoke the "Initial steps" action before the "Checkout repository" step so that it's the first step of the workflow. This applies for all workflows.

@MiguelLZPF
Copy link
Contributor Author

MiguelLZPF commented Oct 22, 2024

@MiguelLZPF as described in this comment - https://github.com/hashgraph/stablecoin-studio/pull/1205/files#r1808173072, please either add the "Hardened runner" step as the first step in the workflows or invoke the "Initial steps" action before the "Checkout repository" step so that it's the first step of the workflow. This applies for all workflows.

@mishomihov00 Done! didn't see the response yesterday, sorry. BTW I have added the "permissions" line to this branch that I have seen changed directly in main over old files and that were generating conflicts in this branch.

mishomihov00
mishomihov00 previously approved these changes Oct 22, 2024
Copy link
Contributor

@mishomihov00 mishomihov00 left a comment

Choose a reason for hiding this comment

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

My review only applies to these files and I'm giving "Approve" only for them:
.github/actions/create-env-file/action.yaml
.github/actions/initial-steps/action.yaml
.github/actions/install-and-build/action.yaml
.github/workflows/all.testWithRpc.yml
.github/workflows/publish-all.backup.yaml
.github/workflows/publish.yaml
.github/workflows/test-backend.yaml
.github/workflows/test-cli.yaml
.github/workflows/test-contracts.yaml
.github/workflows/test-sdk.yaml
.github/workflows/test-web.yaml
.github/workflows/version.yaml

@MiguelLZPF
Copy link
Contributor Author

@mishomihov00 OK, thank you!
@rbarkerSL Only missing your check now 🙂

gregscullard
gregscullard previously approved these changes Oct 23, 2024
Copy link
Collaborator

@gregscullard gregscullard left a comment

Choose a reason for hiding this comment

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

LGTM

@rbarkerSL
Copy link
Contributor

@mishomihov00 OK, thank you! @rbarkerSL Only missing your check now 🙂

Checked again, unresolved the comments. Please put hardened runner ahead of the repo checkout.

@MiguelLZPF
Copy link
Contributor Author

MiguelLZPF commented Oct 24, 2024

@mishomihov00 OK, thank you! @rbarkerSL Only missing your check now 🙂

Checked again, unresolved the comments. Please put hardened runner ahead of the repo checkout.

@rbarkerSL The 'Harden runner' step is now the first step in all the workflows, as you initially pointed out. So, all your requested changes have been addressed. Are you looking at the latest version of the code? Also, it's not mandatory for every action to include this step, correct? If there are any others missing, please let us know so we can resolve them quickly.

We have checked all files again with all the team and should be all OK. As there are a lot of jobs now, if you find one missing please put the comment in the same line so we can see them. Thank you!

Copy link
Contributor

@rbarkerSL rbarkerSL left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants