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.

Sign up for GitHub

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

Jump to bottom

Add check to keep releases.json in sync with the firmware binaries #90

Merged
merged 4 commits into from
Oct 7, 2024
Merged

Add check to keep releases.json in sync with the firmware binaries #90

merged 4 commits into from
Oct 7, 2024

Conversation

peter-sanderson
Copy link
Contributor

@peter-sanderson peter-sanderson commented Aug 7, 2024 •
edited
Loading

This PR adds check to validate presence of the binaries as declared in the releases.json

If some binary is missing:
image

If there is binary but is missing from releases.json
image

It also adds a shellcheck

@peter-sanderson peter-sanderson force-pushed the add-check-to-keep-releases-json-in-sync branch 2 times, most recently from 80d4088 to 3cd5d16 Compare August 7, 2024 11:15
@peter-sanderson peter-sanderson marked this pull request as ready for review August 7, 2024 11:15
peter-sanderson
peter-sanderson commented Aug 7, 2024
View reviewed changes
ci/s3sync.sh Outdated Show resolved Hide resolved
peter-sanderson
peter-sanderson commented Aug 7, 2024
View reviewed changes
ci/s3sync.sh Outdated Show resolved Hide resolved
@peter-sanderson peter-sanderson force-pushed the add-check-to-keep-releases-json-in-sync branch from 3cd5d16 to c551820 Compare August 7, 2024 11:22
komret
komret previously approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@komret komret left a comment

Choose a reason for hiding this comment

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

The shellcheck addition could have been a separate commit.

scripts/run-releases-json-for-all-devices.sh Outdated Show resolved Hide resolved
vdovhanych
vdovhanych requested changes Aug 7, 2024
View reviewed changes
Copy link
Member

@vdovhanych vdovhanych left a comment

Choose a reason for hiding this comment

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

Adding shellcheck validation should be a separate commit and please fix one small nitpick.

.github/workflows/check-shell-validation.yml Outdated Show resolved Hide resolved
ci/s3sync.sh Outdated Show resolved Hide resolved
@peter-sanderson peter-sanderson force-pushed the add-check-to-keep-releases-json-in-sync branch from c551820 to 40773fe Compare August 7, 2024 12:42
@peter-sanderson peter-sanderson force-pushed the add-check-to-keep-releases-json-in-sync branch from 40773fe to b800070 Compare August 8, 2024 10:28
@peter-sanderson
Copy link
Contributor Author

peter-sanderson commented Aug 9, 2024 •
edited
Loading

I have also just found that we have https://github.com/trezor/data/blob/master/check_releases.py

It seems to be checking the existence of binaries for latest release + also checking presence of an changelog , some header checking.

However it checks only Trezor One and Trezor T ❗ . We shall consider unifying those scripts and making it auto-magically work for all devices (when we add new one).

@vdovhanych
Copy link
Member

I have also just found that we have master/check_releases.py

It seems to be checking the existence of binaries for latest release + also checking presence of an changelog , some header checking.

However it checks only Trezor One and Trezor T ❗ . We shall consider unifying those scripts and making it auto-magically work for all devices (when we add new one).

Yes, we have such a script, but that is not true. It is checking t1b1, t2t1, and t2b1. t3t1 is missing from there. I'm not sure how you would make it work automagically, but if you have an idea, go for it.

komret
komret previously approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@komret komret left a comment

Choose a reason for hiding this comment

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

I'd rather see this integrated with check_releases.py, but now that it is done like this, let's use it. @vdovhanych, can you approve?

vdovhanych
vdovhanych previously approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@vdovhanych vdovhanych left a comment

Choose a reason for hiding this comment

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

i guess its fine like this for now

@peter-sanderson peter-sanderson dismissed stale reviews from vdovhanych and komret via b6c74aa October 4, 2024 10:02
@peter-sanderson peter-sanderson force-pushed the add-check-to-keep-releases-json-in-sync branch from 5da6cb6 to b6c74aa Compare October 4, 2024 10:02
@peter-sanderson peter-sanderson force-pushed the add-check-to-keep-releases-json-in-sync branch 4 times, most recently from 8a17862 to 860126b Compare October 7, 2024 11:57
@peter-sanderson peter-sanderson force-pushed the add-check-to-keep-releases-json-in-sync branch 2 times, most recently from 34d7e4c to ca042c9 Compare October 7, 2024 12:25
@peter-sanderson
fix: rework the algorithm so it takes the from releases.json into an …
1c90f9d 
…account, this allows a cross-directory links

fix: binary missing in 2 directory + fix inconsistend url in releases.json

fix: rework the algorithm so it takes the  from releases.json into an account, this allows a cross-directory links
@peter-sanderson peter-sanderson force-pushed the add-check-to-keep-releases-json-in-sync branch from ca042c9 to 1c90f9d Compare October 7, 2024 12:26
Lemonexe
Lemonexe approved these changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@Lemonexe Lemonexe left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@vdovhanych vdovhanych merged commit 9950ae4 into master Oct 7, 2024
4 checks passed
@vdovhanych vdovhanych deleted the add-check-to-keep-releases-json-in-sync branch October 7, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szymonlesisz szymonlesisz szymonlesisz left review comments

@Lemonexe Lemonexe Lemonexe approved these changes

@vdovhanych vdovhanych vdovhanych approved these changes

@komret komret komret left review comments

@MiroslavProchazka MiroslavProchazka Awaiting requested review from MiroslavProchazka

Assignees
No one assigned
Labels
None yet
Projects
Issues Suite
Status: ✅ Approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@peter-sanderson @vdovhanych @szymonlesisz @Lemonexe @komret