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

Fix version check script #1278

Merged
merged 4 commits into from
Jul 18, 2024
Merged

Fix version check script #1278

merged 4 commits into from
Jul 18, 2024

Conversation

area
Copy link
Member

@area area commented Jul 17, 2024

@chmanie 's eagle-eyes noted that it was suspicious that votingReputation had not had its version bumped, but every other extension had. He was right, we should have done as part of #1248 (at least).

I've reworked a fair chunk of the version check script, simply because some of it didn't work. Bash if statements remain a real blind spot for me, but this seems to work.

In summary:

  • Fix iteration over extensions directory by including subdirectories
  • Fix bash if statement
  • Remove 'depends on' logic - I think maybe this was a leftover from when we weren't examining the bytecode directly; if you can come up with a scenario where not having this logic would cause us to miss something, please let me know!
  • Removing that logic meant that some of the other things we're doing with passing variables around become much saner, which is good news.

The version bump means that some of the tests had to be updated, so I've written a upgradeExtensionOnceThenToLatest helper that mirrors the colony equivalent. Not my finest work (an ideal solution would be to look for all relevant ExtensionAddedToNetwork events and take the biggest number), but thanks to my local setup those specific tests for v9 motions are now hard to test locally, and I wanted this out the door to unblock things downstream. If they work locally for you, I'd appreciate you switching out that logic for something event-based. No longer required, managed to do it myself 😄

Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

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

Lgtm

@area area force-pushed the maint/version-check branch from 0c3c458 to bfabec2 Compare July 18, 2024 06:08
@area
Copy link
Member Author

area commented Jul 18, 2024

Curious, how did my rebase not end up dismissing your review? Is it only if the rebase touches files that were previously approved?

@area area merged commit eb3620d into develop Jul 18, 2024
2 checks passed
@area area deleted the maint/version-check branch July 18, 2024 08:15
@kronosapiens
Copy link
Member

kronosapiens commented Jul 18, 2024

Good Q. My guess would be a rebase which does not touch PR-related files doesn't void the review.

More info: https://github.blog/changelog/2023-06-06-security-enhancements-to-required-approvals-on-pull-requests/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants