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.

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: updated instruction for local setup in mac OS #76

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

Subhash703
Copy link
Contributor

Problem

I faced some problems while setting up the project in my Mac OS.

Solution

Have added instructions which I followed to make it work

Environment variable changes

Added extra instruction to update .env file

Pre-deployment activity

Things needed to be done before deploying this change (if any)

Post-deployment activity

Things needed to be done after deploying this change (if any)

API changes

Endpoint Method Request body Response Body
API GET/POST, etc request response

Possible Issues in the future

More instructions can be added in future if needed.

@Subhash703 Subhash703 requested a review from a team as a code owner May 24, 2024 12:37
@Subhash703 Subhash703 force-pushed the fix/setup-instruction branch 2 times, most recently from aef5519 to 7c0dc6e Compare May 24, 2024 12:48
docs/setup.md Outdated
- make sure you have dependencies like libpq, openssl, libiconv, diesel_clietc installed
- To install these dependencies run the following commands -
```bash
brew install libpq # Add libpq to the path
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Subhash703 can we add linux specific instructions too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Datron @Subhash703 - that can come in a separate PR too - because the Linux instructions might depend on distro sometimes. Let us get the Mac instructions merged if they look good.

@Datron Datron added the priority Things that need immediate attention label May 24, 2024
docs/setup.md Outdated
- make sure you have dependencies like libpq, openssl, libiconv, diesel_clietc installed
- To install these dependencies run the following commands -
```bash
brew install libpq # Add libpq to the path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make a single brew command to install all deps to reduce the number of steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Subhash703 Subhash703 force-pushed the fix/setup-instruction branch from 7c0dc6e to a6744b0 Compare May 27, 2024 10:10
@Subhash703 Subhash703 requested a review from Datron May 27, 2024 10:21
docs/setup.md Outdated
- To install these dependencies run the following commands -
```bash
brew install libpq && brew install openssl && brew install libiconv # Add libpq to the path
- Instalstdlib-jdk7l diesel_cli
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Subhash703 - the formatting of the commands seem a little off in the doc.

https://github.com/juspay/superposition/blob/fix/setup-instruction/docs/setup.md

Can we keep all the shell commands listed out separately without indenting them with anything. The jagged grey highlight boxes on the finally rendered doc looks a little odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knutties Could you please check again. Have modified it.

@Subhash703 Subhash703 force-pushed the fix/setup-instruction branch 5 times, most recently from 9db18eb to 8ce4ca4 Compare May 31, 2024 10:51
docs/setup.md Outdated
Comment on lines 20 to 22
Don't forget to add libpq to `PATH`.
```bash
xcode-select --install
export PATH="/usr/local/opt/libpq/bin:$PATH"
Copy link
Member

Choose a reason for hiding this comment

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

For ensuring libpq is picked up by diesel and/or diesel_cli, it's sufficient to set the PQ_LIB_DIR environment variable, setting PATH is not necessary:

export PQ_LIB_DIR="$(brew --prefix libpq)/lib"

Also, would suggest using brew prefix instead of using /usr/local so that the path generated handles architecture differences (Intel vs Apple Silicon).

@Subhash703 Subhash703 force-pushed the fix/setup-instruction branch from 8ce4ca4 to 7756b8b Compare June 3, 2024 11:55
@pratikmishra356
Copy link
Collaborator

@Subhash703 can you rebase this pr

@Subhash703 Subhash703 force-pushed the fix/setup-instruction branch from 7756b8b to e2c6e69 Compare June 5, 2024 12:33
Copy link
Collaborator

@knutties knutties left a comment

Choose a reason for hiding this comment

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

@Subhash703 - I think we can delete this line as well in the environment variable list as we have removed the corresponding sed usage.

https://github.com/juspay/superposition/pull/76/files#diff-4ca53116333428d7d34e6d2a9a64069655c8c37d5d36b856320ac1a0c4cb9b23R130

@ShubhranshuSanjeev @Datron - please confirm. We can then merge I guess.

@Datron Datron requested a review from knutties June 12, 2024 11:18
@mahatoankitkumar mahatoankitkumar merged commit 95205ae into main Jun 12, 2024
4 checks passed
@mahatoankitkumar mahatoankitkumar deleted the fix/setup-instruction branch June 12, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority Things that need immediate attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants