-
Notifications
You must be signed in to change notification settings - Fork 219
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
Fix:2964 obtain Postgres DB URL from ENV using branches get command #2996
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Yaten Dhingra <[email protected]>
@sweatybridge , I've a doubt, that as you mentioned in this comment, that a new Also, as you mentioned, that the postgres_url and prostgres_url_non_pooling has to be added if this flag is mentioned with the command and the code can be reused from the |
Signed-off-by: Yaten Dhingra <[email protected]>
Hey @sweatybridge , could you please have a look at this PR, I've made the required changes, kindly review and LMK if any more are required!! |
Pull Request Test Coverage Report for Build 12609893240Details
💛 - Coveralls |
Signed-off-by: Yaten Dhingra <[email protected]>
Let's do this in smaller steps so I can provide more detailed guidance.
This is how golang works: any variable declared in the same package is shared by all code in that package. Since the output variable is already declared in For your first commit, simply passing it down to branches get command will do. No need to implement the actual behaviour for env output. After you've done that, tag me again so I can check before providing more details for the actual implementation. |
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.
Feel free to open a new PR if it's easier for you.
Also, sorry for the late reply as I was on holiday for the last 2 weeks and there were a lot to catch up on. I will review more promptly this week and next if you plan to make the changes. |
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
Heyy @sweatybridge , sorry, actually, I was pushing some changes right now only and that's why didn't see your comment on this PR.
Also, yes, for this, I was also researching about the same and now I understood that, vars are shared across the same package files in Go. |
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
Co-authored-by: Han Qiao <[email protected]>
Co-authored-by: Han Qiao <[email protected]>
Co-authored-by: Han Qiao <[email protected]>
Now, how should the Run() func be changed in |
Co-authored-by: Han Qiao <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
internal/branches/get/get.go
Outdated
|-|-|-|-|-|-|-| | ||
config := pgconn.Config{ | ||
Host: utils.GetSupabaseDbHost(resp.JSON200.DbHost), | ||
Port: uint16(resp.JSON200.DbPort), |
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.
Port: uint16(resp.JSON200.DbPort), | |
Port: uint16(cast.IntToUint(resp.JSON200.DbPort)), |
To fix lint check
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.
@yaten2302 in case you missed it, there are more linter errors to fix https://github.com/supabase/cli/actions/runs/12608039666
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.
@sweatybridge , yes, ig I forgot about the linter errors, I've a doubt, that in here, these 2 errors are being show
In the 2nd one, should I remove cast.IntToUInt(...)
, I think the error is arising because of that only?
For the first one, I've pushed a commit, I think it should be fixed now (formatted the file)
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.
In the 2nd one, should I remove cast.IntToUInt(...), I think the error is arising because of that only?
We need to implement a new method for safely casting UInt to UInt16 and call it. You can check the existing examples in cast.go on how to check for overflows. But if you feel stuck, I can also take over from here.
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'll research upon it a and then ask if I encounter any doubts! Also, I wanted to ask that, is it possible to check the linter errors are failing or passing without pushing the changes?
Signed-off-by: Yaten Dhingra <[email protected]>
Signed-off-by: Yaten Dhingra <[email protected]>
What kind of change does this PR introduce?
This PR introduces a new feature to enable users to obtain the Postgres db url from the .env using the
branches get
command.What is the current behavior?
Fixes #2964
What is the new behavior?
This PR adds a new flag to the
branches get
command to help users to get the postgres db url from the env.Additional context
Add any other context or screenshots.