Skip to content

09 10 feat sparkles adds xai service#135

Open
alex-klikatech wants to merge 26 commits into
mainfrom
09-10-feat_sparkles_adds_xai_service
Open

09 10 feat sparkles adds xai service#135
alex-klikatech wants to merge 26 commits into
mainfrom
09-10-feat_sparkles_adds_xai_service

Conversation

@alex-klikatech

Copy link
Copy Markdown
Contributor

No description provided.

@bencorrado

Copy link
Copy Markdown

Can we remove Hychain from this PR and make it just the Xai node code?

I think we should probably be merging this to a xai branch too, not to main.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be removed since it's out of scope for Xai.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be removed since it's out of scope for Xai.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be removed since it's out of scope for Xai.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

done

Comment on lines +4 to +6
xai_version: 1.1.14
xai_metrics_port: 33004
xai_download_url: "https://github.com/xai-foundation/sentry/releases/latest/download/sentry-node-cli-linux.zip"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With the depin.core.installer the following should be updated.

xai_version: 1.1.14
xai_download_urls: 
  - "https://github.com/xai-foundation/sentry/releases/latest/download/sentry-node-cli-linux.zip"

The following should be moved to depin/core/roles/metrics/default.yml

xai_metrics_port: 33004

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

done

xai_version: 1.1.14
xai_metrics_port: 33004
xai_download_url: "https://github.com/xai-foundation/sentry/releases/latest/download/sentry-node-cli-linux.zip"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

depin.core.installer is expecting the variable xai_programs which is a list of all binaries that're downloaded and then installed

xai_programs
  - [name of binary]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To configure this part correctly - need to receive license key and test wallet. Only after that XAI can be correctly configured, tested and then automated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing command

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can't do this without test license keys and wallet

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing command

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can't do this without test license keys and wallet

Comment thread depin/services/roles/xai/tasks/main.yml Outdated
Comment thread molecule/default/tasks/vars/hychain.yml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The use of this file was removed on a recent breaking_change that was pushed. The hychain_private_key is being pulled and set via Pulumi ESC.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok

Comment thread skeletons/role/tasks/metrics.yml.j2 Outdated
Comment thread depin/core/roles/metrics/default.yml Outdated
@@ -0,0 +1,2 @@
---
xai_metrics_port: 33004

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The xai_metrics_port value of 33004 conflicts with the storagechain port assignment in defaults/main.yml. To maintain consistency with the metrics configuration, this value should be 33005.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

done

Comment on lines +1 to +2
---
# Run the XAI Sentry Node CLI

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The start.yml task appears to be missing the implementation to start the XAI service. The xai_start_command variable is defined in defaults/main.yml but not utilized here. Consider adding a task to execute this command, likely using the ansible.builtin.command module.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we have no license keys

Comment on lines +1 to +2
---
# Stop XAI Sentry Node service

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The stop.yml task appears to be missing the implementation to stop the XAI service. Consider adding a task that uses the xai_stop_command variable defined in defaults/main.yml to properly terminate the service.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the same - without license keys impossible to write and check start/stop

xai_programs:
- name: sentry-node-cli-linux
url: "{{ xai_download_url }}"
checksum: ""

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The checksum field is empty, which bypasses an important security check. Adding a SHA256 checksum for the XAI Sentry Node binary would protect against supply chain attacks and ensure binary integrity during downloads. Consider obtaining the official checksum from the XAI Foundation release page.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@skuznetsov-klika

Copy link
Copy Markdown

Can we remove Hychain from this PR and make it just the Xai node code?

I think we should probably be merging this to a xai branch too, not to main.

Done

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