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.

Sign up for GitHub

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

Jump to bottom

nostr script gridclient #3776

Open
wants to merge 6 commits into
base: development
Choose a base branch
from
Open

nostr script gridclient #3776

wants to merge 6 commits into from

Conversation

khaledyoussef24
Copy link
Contributor

Description

creating nostr solution script using gridclient
#3330

Describe the changes introduced by this PR and what does it affect

Changes

List of changes this PR includes

Related Issues

List of related issues

Tested Scenarios

A list of scenarios tried to match the deliverables

Documentation PR

For UI changes, Please provide the Documentation PR on info_grid

To consider

Preliminary Checks:

  • Preliminary Checks
    • Does it completely address the issue linked?
    • What about edge cases?
    • Does it meet the specified acceptance criteria?
    • Are there any unintended side effects?
    • Does the PR adhere to the team's coding conventions, style guides, and best practices?
    • Does it integrate well with existing features?
    • Does it impact the overall performance of the application?
    • Are there any bottlenecks or slowdowns?
    • Has it been optimized for efficiency?
    • Has it been adequately tested with unit, integration, and end-to-end tests?
    • Are there any known defects or issues?
    • Is the code well-documented?
    • Are changes to documentation reflected in the code?

UI Checks:

  • UI Checks
    • If a UI design is provided/ does it follow it?
    • Does every button work?
    • Is the data displayed logical? Is it what you expected?
    • Does every validation work?
    • Does this interface feel intuitive?
    • What about slow network? Offline?
    • What if the gridproxy/graphql/chain is failing?
    • What would a first time user have a hard time navigating here?

Code Quality Checks:

  • Code Quality Checks
    • Code formatted/linted? Are there unused imports? .. etc
    • Is there redundant/repeated code?
    • Are there conditionals that are always true or always false?
    • Can we write this more concisely?
    • Can we reuse this code? If yes, where?
    • Will the changes be easy to maintain and update in the future?
    • Can this code become too complex to understand for other devs?
    • Can this code cause future integration problems?

Testing Checklist

  • Does it Meet the specified acceptance criteria.
  • Test if changes can affect any other functionality.
  • Tested with unit, integration, and end-to-end tests.
  • Tested the un-happy path and rollback scenarios.
  • Documentation updated to meet the latest changes.
  • Check automated tests working successfully with the changes.
  • Can be covered by automated tests.

General Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring
  • Screenshots/Video attached (needed for UI changes)

@khaledyoussef24 khaledyoussef24 marked this pull request as ready for review January 13, 2025 13:39
0oM4R
0oM4R requested changes Jan 13, 2025
View reviewed changes
packages/grid_client/scripts/applications/nostor.ts
import { config, getClient } from "../client_loader";
import { log, pingNodes } from "../utils";

async function deploy(client: GridClient, vms: MachinesModel, subdomain: string, gatewayNode: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
async function deploy(client: GridClient, vms: MachinesModel, subdomain: string, gatewayNode: any) {
async function deploy(client: GridClient, vms: MachinesModel, subdomain: string, gatewayNode: NodeInfo) {

please update this type its better to avoid any

Copy link
Contributor

Choose a reason for hiding this comment

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

The gatewayNode's type is still any, please change it

Copy link
Contributor

Choose a reason for hiding this comment

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

please make sure to change the type here

@khaledyoussef24 khaledyoussef24 force-pushed the development-nostr-deployment branch from 437e7fd to bb1cc9f Compare January 22, 2025 17:34
maayarosama
maayarosama requested changes Jan 30, 2025
View reviewed changes
packages/grid_client/scripts/applications/nostor.ts
import { config, getClient } from "../client_loader";
import { log, pingNodes } from "../utils";

async function deploy(client: GridClient, vms: MachinesModel, subdomain: string, gatewayNode: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The gatewayNode's type is still any, please change it

packages/grid_client/scripts/applications/nostor.ts Outdated Show resolved Hide resolved
packages/grid_client/scripts/applications/nostor.ts Show resolved Hide resolved
@khaledyoussef24
Copy link
Contributor Author

Changes and suggestions made and resolved

0oM4R
0oM4R requested changes Feb 4, 2025
View reviewed changes
packages/grid_client/scripts/applications/nostor.ts
import { config, getClient } from "../client_loader";
import { log, pingNodes } from "../utils";

async function deploy(client: GridClient, vms: MachinesModel, subdomain: string, gatewayNode: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please make sure to change the type here

packages/grid_client/scripts/applications/nostor.ts

// Gateway Query Options
const gatewayQueryOptions: FilterOptions = {
features: [Features.wireguard, Features.mycelium, Features.gatewaynameproxy],
Copy link
Contributor

Choose a reason for hiding this comment

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

the only feature needed here is wireguard; please remove the others.
the gateway features are added internally in node capacitor as long as we have the gateway= true

packages/grid_client/scripts/applications/nostor.ts
availableFor: grid3.twinId,
};

const gatewayNode = (await grid3.capacity.filterNodes(gatewayQueryOptions))[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use the ping node here to same as the vm node; get the full resopvense and pass it to ping node

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maayarosama maayarosama maayarosama requested changes

@0oM4R 0oM4R 0oM4R requested changes

@AhmedHanafy725 AhmedHanafy725 Awaiting requested review from AhmedHanafy725 AhmedHanafy725 is a code owner

@zaelgohary zaelgohary Awaiting requested review from zaelgohary zaelgohary is a code owner

@MohamedElmdary MohamedElmdary Awaiting requested review from MohamedElmdary MohamedElmdary is a code owner

@Mahmoud-Emad Mahmoud-Emad Awaiting requested review from Mahmoud-Emad Mahmoud-Emad is a code owner

@amiraabouhadid amiraabouhadid Awaiting requested review from amiraabouhadid amiraabouhadid is a code owner

@AlaaElattar AlaaElattar Awaiting requested review from AlaaElattar AlaaElattar is a code owner

@samaradel samaradel Awaiting requested review from samaradel samaradel is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@khaledyoussef24 @0oM4R @maayarosama