-
Notifications
You must be signed in to change notification settings - Fork 72
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
Bump crystal-ameba/ameba to 1.5.0 #1811
Conversation
3676c32
to
711dc14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will verify and test before merging.
|
Hello @rsvoboda , Yes, our build process requires some other authentication with Docker Hub. I'll be doing a checkout and check-in of your source to verify our github actions pass before any merges. Thanks. |
Also to note, we don't officially support Mac for the testsuite but I plan to test both on Mac and our regular build process to ensure there's no other breakage with this crystal module update. Thanks. |
Getting the following error on Linux using our supported crystal version 1.6.0:
We'll likely have to bump crystal version as well to jump this far with ameba on the latest version. @rsvoboda , have you tried using crystal version 1.6.0 on MacOS and ameva 1.3.0 by chance? We tend to update crystal and it's modules on a quarterly basis which the next bump should be around the corner. Thanks for any input and for submitting this PR, we'll get there. |
Hi @agentpoyo. I used Brew to install crystal (https://formulae.brew.sh/formula/crystal) so that's why I ended up with 1.9.2. https://github.com/cncf/cnf-testsuite/blob/main/SOURCE_INSTALL.md mentions that crystal-lang version should be |
Hello @rsvoboda , Thanks for confirming that crystal 1.6.0 compiles without issues on MacOS. That verbiage needs to be updated since future crystal versions could break things before they are fully vetted and tested. I've created a new issue to get crystal upgraded (modules will follow suit as well), you can find that here: https://github.com/cncf/cnf-testsuite/issues/1814 I also believe that .crystal-version file is not used but I'll need to verify. Thanks again! |
I can confirm that # NOTE: Linux only
curl -fsSL https://crystal-lang.org/install.sh | sudo bash -s -- --version=1.6 The command above is from the Crystal site and installs the latest 1.6.x. You can replace it with 1.9 and it will work as expected. |
@agentpoyo Just to confirm, we need to hold off on merging this PR. I looked up ameba's shard.yml and it requires crystal 1.9. @rsvoboda The testsuite is always pinned to a specific minor version of Crystal as a requirement. For Mac, to install a specific version, the crystal website mentions using the github releases as the only way. Below is the link to the 1.6.2 release. |
Fixes issue with shards install in macOS Signed-off-by: Rostislav Svoboda <[email protected]>
711dc14
to
e2ebbd8
Compare
Tried to rebase to the latest, compilation fails. Not worth the effort as concrete version of Crystal is needed |
The issue is still very relevant, being discussed on community meetings and in the scope of #2156. I'm planning to gather knowledge about all needed changes to make this update. |
This is an automated update of a crystal dependency
WARNING: this requires an upgrade to Crystal. See comments below #1811 (comment)
Bump crystal-ameba/ameba to 1.5.0
Fixes issue with shards install in macOS
Crystal version
Error message:
Types of changes:
Checklist:
Documentation