-
Notifications
You must be signed in to change notification settings - Fork 1
Master ewf deployer #95
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
Conversation
backend/app/app.go
Outdated
| // deployer.WithSubstrateURL("wss://tfchain.dev.grid.tf/ws"), | ||
| // deployer.WithProxyURL("https://gridproxy.dev.grid.tf"), | ||
| // deployer.WithRelayURL("wss://relay.dev.grid.tf"), | ||
| deployer.WithSubstrateURL("wss://tfchain.dev.grid.tf/ws"), |
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.
Remember to remove after testing
backend/kubedeployer/client_test.go
Outdated
| ctx := context.Background() | ||
|
|
||
| cluster := Cluster{ | ||
| Name: "jrk8s12", |
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.
definitely not jerk xD
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.
i meant jarvis-kubernetes-# :D
chaned
backend/kubedeployer/client.go
Outdated
| gridNet string | ||
| mnemonic string | ||
| masterPubKey string | ||
| UserID string |
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.
Why part of the client while that can get passed as an argument?
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.
just easier to set/get, used in multiple functions
- move the cancel/remove code from handler to the pkg - modify to client struct - introduce naming convension - better testing
301894b to
fe3260c
Compare
| }, | ||
| } | ||
|
|
||
| func registerDeploymentActivities(engine *ewf.Engine) { |
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.
Please make all strings constants to be used anywhere
| if err != nil { | ||
| log.Error().Err(err).Str("user_id", userIDStr).Msg("Failed to parse user ID") | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to parse user ID"}) | ||
| var cluster kubedeployer.Cluster |
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.
I think this should be the first check in function
backend/app/deployment_handler.go
Outdated
| TaskID: wf.UUID, | ||
| Status: string(wf.Status), | ||
| Message: "Node addition workflow started successfully", | ||
| CreatedAt: wf.CreatedAt, |
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.
why u are returning CreatedAt ?
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to update deployment in database"}) | ||
| wf, err := h.ewfEngine.NewWorkflow("remove_node") | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to create workflow"}) |
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.
There's already an InternalServerError(c) function defined to be used.
backend/app/deployment_handler.go
Outdated
| "deployment_name": deploymentName, | ||
| "node_name": nodeName, | ||
| c.JSON(http.StatusOK, Response{ | ||
| TaskID: wf.UUID, |
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.
its not a task id, it is a workflow id.
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.
Also there's endpoint for checking the workflow status by its id. u don't have to return status here
| return fmt.Errorf("failed to assign node IPs for added cluster: %w", err) | ||
| } | ||
|
|
||
| for idx, node := range addedCluster.Nodes { |
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.
what if smth failed in middle what will happen when workflow engine retries? will it start the loop from beginning ?
backend/cmd/root.go
Outdated
|
|
||
| // === Workflow DB File === | ||
| if err := bindStringFlag(rootCmd, "workflow_db_file", "", "Workflow database file path"); err != nil { | ||
| return fmt.Errorf("failed to bind workflow_db_file flag: %w", err) |
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.
u deleted workflow_db_file from Configuration struct, u should delete it from here too.
| } | ||
| engine.RegisterTemplate("remove_cluster", &deleteWFTemplate) | ||
|
|
||
| // addNodeWFTemplate := baseWFTemplate |
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.
please remove commented code
backend/kubedeployer/client.go
Outdated
| var nodeToRemove *Node | ||
| var nodeIndex int | ||
| for i, node := range cluster.Nodes { | ||
| fmt.Println("full node name:", fullNodeName) |
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.
why fmt ? the app already uses logger
backend/app/deployment_handler.go
Outdated
| log.Debug().Str("user_id", userIDStr).Str("deployment_name", deploymentName).Msg("Starting deployment deletion") | ||
|
|
||
| cluster, err := h.db.GetClusterByName(userIDStr, deploymentName) | ||
| // TODO: load it at startup instead |
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.
Fix the todo please
Omarabdul3ziz
left a comment
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.
eh el3zma de
Description
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
Checklist