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.

Sign up for GitHub

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

Jump to bottom

Improved Quickstart Guide #246

Merged
merged 1 commit into from
May 24, 2021
Merged

Improved Quickstart Guide #246

merged 1 commit into from
May 24, 2021

Conversation

boramalper
Copy link
Contributor

@boramalper boramalper commented May 21, 2021 •
edited
Loading

Improved the Quickstart Guide as discussed today. I have tested the multinode "Serverless (KNative) Cluster" setup extensively and the instructions work well.

@MBaczun Perhaps you would like to give your final feedback too?

@MBaczun
Copy link
Contributor

MBaczun commented May 21, 2021

I think this looks great, I find it a lot more readable than the original too. With the addition of the logs some of the commands are slightly intimidating in their length, but I don't think this is an issue. I feel that numbering the steps and explicitly stating some of the assumptions (even the simple stuff like adding a cd vhive step) really helps the readability, and I'm a big fan of the way you've done the notes. I mentioned the slight repetition to you earlier and you managed to remove that too which is helpful for the reader I think. Overall no complaints from me :)

ustiugov
ustiugov requested changes May 21, 2021
View reviewed changes
Copy link
Member

@ustiugov ustiugov left a comment

Choose a reason for hiding this comment

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

An excellent start, @boramalper! please see my comments. Please fix the linter check failure after we finalize the text.

docs/quickstart_guide.md Show resolved Hide resolved
docs/quickstart_guide.md Outdated Show resolved Hide resolved
docs/quickstart_guide.md Outdated Show resolved Hide resolved
docs/quickstart_guide.md Show resolved Hide resolved
docs/quickstart_guide.md Show resolved Hide resolved
docs/quickstart_guide.md Outdated Show resolved Hide resolved
@ustiugov
Copy link
Member

@boramalper @MBaczun please avoid using "merge" commits. To update your branch, you need to use git rebase vs the main branch.

image

as of now, please squash the last 2 commits into a single one in this branch, using git rebase.

@boramalper
Copy link
Contributor Author

image

as of now, please squash the last 2 commits into a single one in this branch, using git rebase.

I haven't used git rebase retrospectively much, please let me know if I did it right. If so, I will proceed making the changes that you have requested.

ustiugov
ustiugov requested changes May 21, 2021
View reviewed changes
Copy link
Member

@ustiugov ustiugov left a comment

Choose a reason for hiding this comment

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

minor changes. also, the branch needs to be rebased vs upstream/main (see my comments on Slack)

docs/quickstart_guide.md Outdated Show resolved Hide resolved
docs/quickstart_guide.md Outdated Show resolved Hide resolved
@ustiugov
Copy link
Member

ustiugov commented May 22, 2021 •
edited by boramalper
Loading

LGTM

Please fix the linter check:

  • check spelling errors
  • add exceptions to the configs/.word* file

@ustiugov
Copy link
Member

@yl3469 would be great to hear your feedback

@boramalper
Copy link
Contributor Author

Please fix the linter check:

  • check spelling errors

  • add exceptions to the configs/.word* file

It marked the following as spelling errors:

 Misspelled words:
<htmlcontent> docs/quickstart_guide.md: html>body>p
--------------------------------------------------------------------------------
dmS
sha
stderr
stdout
tmp
--------------------------------------------------------------------------------

All of which exist in markdown codeblocks. Should I modify .spellcheck.yml as follows instead?

matrix:
- name: Markdown
  aspell:
    lang: en
  dictionary:
    wordlists:
    - ./configs/.wordlist.txt
    encoding: utf-8
  pipeline:
  - pyspelling.filters.markdown:
    ignores:
    - code
    - pre
  - pyspelling.filters.html:
    comments: false
    ignores:
    - code
    - pre
  sources:
  - '**/*.md'
  default_encoding: utf-8

Or do you have other suggestions?

@ustiugov
Copy link
Member

ustiugov commented May 22, 2021 via email

@boramalper
Improved Quickstart Guide
14b21d8 
Signed-off-by: Bora M. Alper <[email protected]>
@ustiugov
Copy link
Member

looks good, let's see if we can get any feedback from other users

@MBaczun can you please check both the multi-node and the single node guide on Cloudlab?

@ustiugov ustiugov requested a review from MBaczun May 23, 2021 14:06
@MBaczun
Copy link
Contributor

MBaczun commented May 24, 2021

I have tried out both methods on cloudlab and everything works for me :)

@boramalper boramalper merged commit ea634bc into vhive-serverless:main May 24, 2021
@boramalper boramalper deleted the new-quickstart branch May 24, 2021 11:18
HermioneKT added a commit that referenced this pull request Jan 31, 2024
@HermioneKT
# This is a combination of 252 commits.
3c6919f 
parent 6674807
author HermioneKT <[email protected]> 1706694164 +0800
committer HermioneKT <[email protected]> 1706694164 +0800

test

# This is the commit message #157:

test

# This is the commit message #159:

test

# This is the commit message #160:

test

# This is the commit message #161:

test

# This is the commit message #162:

test

# This is the commit message #163:

test

# This is the commit message #164:

tesT

# This is the commit message #165:

test

# This is the commit message #166:

test

# This is the commit message #167:

test

# This is the commit message #168:

test

# This is the commit message #169:

test

# This is the commit message #170:

test

# This is the commit message #171:

Test

# This is the commit message #172:

test

# This is the commit message #173:

test

# This is the commit message #174:

test

# This is the commit message #175:

test

# This is the commit message #176:

test

# This is the commit message #177:

test

# This is the commit message #178:

test

# This is the commit message #179:

test

# This is the commit message #180:

test

# This is the commit message #181:

test

# This is the commit message #182:

test

# This is the commit message #183:

test

# This is the commit message #184:

test

# This is the commit message #185:

test

# This is the commit message #186:

test

# This is the commit message #187:

test

# This is the commit message #188:

test

# This is the commit message #189:

test

# This is the commit message #190:

Test

# This is the commit message #191:

Test

# This is the commit message #192:

test

# This is the commit message #193:

Test

# This is the commit message #194:

test

# This is the commit message #195:

test

# This is the commit message #196:

test

# This is the commit message #197:

test

# This is the commit message #198:

test

# This is the commit message #199:

Test

# This is the commit message #200:

test

# This is the commit message #201:

test

# This is the commit message #202:

Test

# This is the commit message #203:

test

# This is the commit message #204:

test

# This is the commit message #205:

test

# This is the commit message #206:

test

# This is the commit message #207:

test

# This is the commit message #208:

test

# This is the commit message #209:

test

# This is the commit message #210:

test

# This is the commit message #211:

Test

# This is the commit message #212:

test

# This is the commit message #213:

Test

# This is the commit message #214:

Test

# This is the commit message #215:

Test

# This is the commit message #216:

test

# This is the commit message #217:

Test

# This is the commit message #218:

test

# This is the commit message #219:

test

# This is the commit message #220:

test

# This is the commit message #221:

test

# This is the commit message #222:

test

# This is the commit message #223:

test

# This is the commit message #224:

test

# This is the commit message #225:

test

# This is the commit message #226:

test

# This is the commit message #227:

test

# This is the commit message #228:

test

# This is the commit message #229:

Test

# This is the commit message #230:

test

# This is the commit message #231:

test

# This is the commit message #232:

test

# This is the commit message #233:

test

# This is the commit message #234:

Test

# This is the commit message #235:

test

# This is the commit message #236:

test

# This is the commit message #237:

test

# This is the commit message #238:

test

# This is the commit message #239:

test

# This is the commit message #240:

test

# This is the commit message #241:

test

# This is the commit message #242:

test

# This is the commit message #243:

test

# This is the commit message #244:

test

# This is the commit message #245:

test

# This is the commit message #246:

test

# This is the commit message #247:

test

# This is the commit message #248:

test

# This is the commit message #249:

test

# This is the commit message #250:

test

# This is the commit message #251:

test

# This is the commit message #252:

test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ustiugov ustiugov ustiugov approved these changes

@MBaczun MBaczun MBaczun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@boramalper @MBaczun @ustiugov