feat: Improvement over health API #143
Conversation
|
Please make sure to define a new env variable called SHUFFLE_BACKEND_VERSION to get the records in health api logs. |
health.go
Outdated
| @@ -893,18 +885,57 @@ func fixOpensearch() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func FixSubflow(ctx context.Context, workflowId string) error { | |||
There was a problem hiding this comment.
I'm sorry, what is happening here? Why do we have a custom fixSubflow function in the API, which SETS the workflow in the database differently?
If you were just returning the workflow to be used in the parent it's ok, but SETTING, then GETTING it again doesn't really do that.
Please explain. This makes no sense to me
There was a problem hiding this comment.
I had to set it this way because we use the API call to the <workflowID>/execute endpoint to run the workflow. So, if I modify any field in the workflow, I need to save the workflow. However, you're right—there is a redundant call to GetWorkflow, which I will fix.
I also just realized that instead of using the SetWorkflow function, I should use the API. While we know that SetWorkflow works, we are not actually verifying whether sending JSON to the save API functions correctly (which should test both the SetWorkflow as well as json handling). I'll update the code accordingly.
|
Should we merge this? Is it ready? |
Send a PR with this updated in every single one. This NEEDS to be dynamic. |
|
Can we track the version with the deployment time. It will be pretty easy to implement onto the deployment script, so don't have to update the version manually and we can track it with the deployment date. |
|
New Things Added:
|
|
Next PRs will focus on using the gcloud to track logs which can be interesting. |
That is literally what I said. Dynamic. Just use the revision thing Aditya already made. |
health.go
Outdated
| @@ -893,18 +885,104 @@ func fixOpensearch() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func fixSubflow(ctx context.Context, workflow *Workflow) error { | |||
There was a problem hiding this comment.
Make the name of the function more specific please. This tells me nothing 😆
There was a problem hiding this comment.
Done! Changed something which make it more specific.
health.go
Outdated
| } | ||
| } | ||
|
|
||
| baseUrl := os.Getenv("SHUFFLE_CLOUDRUN_URL") |
There was a problem hiding this comment.
Write these opposite. Again; SANE DEFAULTS.
Set default to shuffler.io -> override with cloudrun URL
Further; does this code run onprem? Is this handled then, as to point to the right system?
There was a problem hiding this comment.
Done!
Handled the on-prem case as well, tested and working.
health.go
Outdated
| baseUrl := os.Getenv("SHUFFLE_CLOUDRUN_URL") | ||
| if len(baseUrl) == 0 { | ||
| baseUrl := "https://shuffler.io" | ||
| if len(os.Getenv("SHUFFLE_CLOUDRUN_URL")) > 0 { |
There was a problem hiding this comment.
Always use SHUFFLE_CLOUDRUN_URL last, as that makes it possible to use it as an onprem environment variable to override URLs in general.
health.go
Outdated
| @@ -893,18 +885,113 @@ func fixOpensearch() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func fixSubflowParameters(ctx context.Context, workflow *Workflow, apiKey string, orgId string) error { | |||
There was a problem hiding this comment.
Is this a generic function we can pass in a workflow into and it will fix parameters at all times, or is it specifically for health?
If it's for health, please make that clear in the name, as while grepping for relevant functions, I may end up just using it (:
There was a problem hiding this comment.
It is specifically for the health API. Let me fix the name of the functions to make them more clear :)
health.go
Outdated
| baseUrl = os.Getenv("SHUFFLE_CLOUDRUN_URL") | ||
| } | ||
|
|
||
| req, err := http.NewRequest("PUT", baseUrl+"/api/v1/workflows/"+workflow.ID+"?skip_save=true", nil) |
There was a problem hiding this comment.
Make this more readable. Just make a variable for the URL and newline each item. Hard to come in here and get the point without that.
Why is it a PUT request without any data? 🤔
There was a problem hiding this comment.
We are actually defining the body in the later part of the code, this is just being use to create new Request type.
https://github.com/Shuffle/shuffle-shared/pull/143/files/a9d13e1dae8f0fa3863f10e982a4191dacf46c2f#diff-46ab8d7c52533584f96ba53eaeadd8cf52839f1c5d175c059ceb909c2c3e0715R947
|
Hi @frikky, Please review :) Will push other features in a different PR. |
No description provided.