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

Node/dev aarzola alpine fix #1287

Closed

Conversation

alex-arzola-imp
Copy link
Contributor

Issue #, if available: 1205

Description of changes:
Testing the Alpine Linux to find out where we stand

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@alex-arzola-imp alex-arzola-imp requested a review from a team as a code owner April 15, 2024 20:09
named_os:
description: "The name of the current operating system"
required: false
default: "linux"
type: string
options:
- linux
- linux-musl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see the comments I wrote on the other PR - we shouldn't use linux-musl here

- darwin
arch:
description: "The current architecture"
required: false
default: "x64"
default: "arm64"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why? don't change the default

os: "ubuntu-latest-musl"
target: "aarch64-unknown-linux-musl"
github-token: ${{ secrets.GITHUB_TOKEN }}
arch: "x64"
Copy link
Collaborator

Choose a reason for hiding this comment

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

arch should be arm64

@@ -54,6 +56,14 @@ jobs:
TARGET: aarch64-unknown-linux-gnu,
CONTAINER: "2_28",
}
- {
OS: ubuntu-latest-musl,
NAMED_OS: linux-musl,
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to "linux"

@@ -12,6 +12,7 @@ inputs:
type: string
options:
- linux
- linux-musl
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

@@ -17,7 +18,9 @@ function loadNativeBinding() {
nativeBinding = require("@scope/glide-for-redis-linux-x64");
break;
case "arm64":
nativeBinding = require("@scope/glide-for-redis-linux-arm64");
nativeBinding = existsSync("./node_modules/@scope/glide-for-redis-linux-musl-arm64")
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to check with isMusl

arch: "x64"
named_os: "linux"

- name: Create a symbolic Link for redis6 binaries
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to step in the alpine image? it's relevant to amazon-linux which installs redis as redis6

@acarbonetto acarbonetto self-assigned this Apr 30, 2024
@barshaul
Copy link
Collaborator

barshaul commented May 2, 2024

Will continue to keep track in #1267

@barshaul barshaul closed this May 2, 2024
@Yury-Fridlyand Yury-Fridlyand deleted the node/dev_aarzola_alpine_fix branch July 19, 2024 02:53
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.

4 participants