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 few VM issues within Quick_start_for_developing.md #463

Closed
wants to merge 3 commits into from

Conversation

aaryapatel100
Copy link

No description provided.

@DanilYachmenev
Copy link
Collaborator

@aaryapatel100 what issue is this related to btw?
I expect this is a follow-up from the onboarding, right?

if so, pls put a reference to it in the PR desc
also pls read https://github.com/sorrentum/sorrentum/blob/master/docs/First_review_process.md to see how to treat the PR to make sure that it is not unintentionally overlooked

@DanilYachmenev DanilYachmenev added the PR_for_reviewers The PR needs to be reviewed by Team Leaders label Jul 27, 2023
Copy link
Collaborator

@samarth9008 samarth9008 left a comment

Choose a reason for hiding this comment

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

Although, I have reviewed but lets wait for @DanilYachmenev comment before implementing any changes.

docs/Quick_start_for_developing.md Outdated Show resolved Hide resolved
docs/Quick_start_for_developing.md Outdated Show resolved Hide resolved
docs/Quick_start_for_developing.md Outdated Show resolved Hide resolved
@DanilYachmenev
Copy link
Collaborator

Although, I have reviewed but lets wait for @DanilYachmenev comment before implementing any changes.

honestly, I'm a bit out of context here
@aaryapatel100 could you extrapolate, what this changes are related to at all?

@aaryapatel100
Copy link
Author

Yeah, so these were all issues/errors that I encountered in the onboarding process in which I thought the steps might be better clarified in the documentation. Important to note, that these did come up within VMWare usage so not sure if errors will replicate for all but in the case that they do easy things to look out for. The aws-cli installation error during thin environment build is probably the most notable one as it just crashed the terminal in less than a second of outputting error and seems to be a much larger issue with this specific package dependency. I figured that adding a note about terminal crashing would be useful for 2 reasons: a) saves time for onboarder from having to screen record build process and see what happened and b) given the speed of crash, someone might figure it's not necessarily an error, open terminal, run the activation command, and deal with a whole host of other issues.

@DanilYachmenev
Copy link
Collaborator

ok then implement the comments that Samarth has left and we can ask for GP to take a look

@DanilYachmenev DanilYachmenev added PR_for_authors The PR needs changes and removed PR_for_reviewers The PR needs to be reviewed by Team Leaders labels Jul 28, 2023
@aaryapatel100
Copy link
Author

Ok, comments have been implemented. @gpsaggese Could you please take a look to the updated quick-start documentation?

@samarth9008 samarth9008 added PR_for_reviewers The PR needs to be reviewed by Team Leaders and removed PR_for_authors The PR needs changes labels Aug 1, 2023
@samarth9008 samarth9008 added PR_for_integrators The PR needs to be reviewed and possibly merged and removed PR_for_reviewers The PR needs to be reviewed by Team Leaders labels Aug 3, 2023
@samarth9008
Copy link
Collaborator

Closing as not planned.

@samarth9008 samarth9008 closed this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR_for_integrators The PR needs to be reviewed and possibly merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants