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 presets page update on release workflow #883

Merged
merged 23 commits into from
Feb 23, 2024

Conversation

ericnordelo
Copy link
Member

No description provided.

@ericnordelo ericnordelo changed the title Add update class hashes workflow Add Check class hashes workflow Jan 31, 2024
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Excellent work, Eric! I left a few comments. And +1 to the offline discussion with having this check only on releases

.github/workflows/class-hash.yml Outdated Show resolved Hide resolved
.github/workflows/class-hash.yml Outdated Show resolved Hide resolved
.github/workflows/class-hash.yml Outdated Show resolved Hide resolved
.github/actions/check-docsite-class-hashes/index.js Outdated Show resolved Hide resolved
.github/actions/check-docsite-class-hashes/index.js Outdated Show resolved Hide resolved
.github/actions/check-docsite-class-hashes/index.js Outdated Show resolved Hide resolved
.github/actions/check-docsite-class-hashes/index.js Outdated Show resolved Hide resolved
@ericnordelo ericnordelo changed the title Add Check class hashes workflow Add presets page update on release workflow Feb 8, 2024
@ericnordelo
Copy link
Member Author

I removed the local javascript action and addressed Andrew's comments, but I found myself spending an important amount of time developing the action in bash, so I just created a python script for it. I don't think it is worth spending the time on implementing what the python script does directly in bash. Let me know if you think otherwise.

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Nice work! This looks way better and easier to maintain than the js action IMO. I left a few small suggestions

scripts/get_presets_page.py Outdated Show resolved Hide resolved
.github/workflows/prepare-release.yml Outdated Show resolved Hide resolved
scripts/get_presets_page.py Outdated Show resolved Hide resolved
scripts/get_presets_page.py Outdated Show resolved Hide resolved
.github/workflows/prepare-release.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

LGTM!

@ericnordelo ericnordelo linked an issue Feb 14, 2024 that may be closed by this pull request
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

it looks great!

Comment on lines 6 to 7
env:
SCARB_VERSION: 2.5.3
Copy link
Contributor

Choose a reason for hiding this comment

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

what's our strategy to maintain this up to date?

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea is to continue using the same we've been using: update this version at the same time we update the Scarb.toml. We had to do this already for the lint_and_test action.

Copy link
Contributor

@martriay martriay Feb 16, 2024

Choose a reason for hiding this comment

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

i see. does it make sense to automate it in some way? is there something preventing us from mistakenly not updating it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm under the impression it might be easy to forget about this bump and mistakenly publish the wrong hashes. what about reading it from Scarb.toml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

looks great!

.github/workflows/prepare-release.yml Outdated Show resolved Hide resolved
@ericnordelo ericnordelo merged commit e0e888c into OpenZeppelin:main Feb 23, 2024
5 checks passed
@ericnordelo ericnordelo deleted the feat/class-hash-action branch February 23, 2024 11:28
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.

Update presets page workflow
3 participants