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

[#12444] Updated development.md #12525

Merged
merged 14 commits into from Jul 26, 2023
Merged

Conversation

Rajdeep1311
Copy link
Contributor

Fixes #12444

Outline of Solution
Provided a clear solution for those using v1 and v2 of docker respectively.

@weiquu
Copy link
Contributor

weiquu commented Jul 21, 2023

Hi @Rajdeep1311, some quick suggestions:

  • Perhaps we should swap the 2 commands around (i.e. put docker compose command first) since that is the direction Docker is going towards. Then, we can suggest the alternate docker-compose command if the first one doesn't work
  • For external URLs, let's add a hyperlink to some text, so for example "For more information, you may refer to the Docker documentation"

@Rajdeep1311
Copy link
Contributor Author

Okay I will be updating that

@weiquu
Copy link
Contributor

weiquu commented Jul 22, 2023

Thanks @Rajdeep1311 for the changes! There's another reference to docker-compose that seems to have been missed out. It's on line 246, under the "Running the Datastore emulator" section:
Screenshot 2023-07-23 at 1 55 10 AM

Could you add something similar here? Just edit the command to docker compose and add a line under telling them to use docker-compose instead if the first command doesn't work

@Rajdeep1311
Copy link
Contributor Author

Sure

Copy link
Contributor

@jasonqiu212 jasonqiu212 left a comment

Choose a reason for hiding this comment

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

Hi @Rajdeep1311, Thanks for the changes! Just some quick suggestions:

  1. As per my comments, I suggested some changes on the phrasing. Do you think they improve the clarity of the documentation?
  2. I believe there's another reference to docker-compose in search.md on line 19.
Screen Shot 2023-07-23 at 2 56 27 PM

Like your previous changes, could you add something similar here?

docs/development.md Outdated Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
@weiquu
Copy link
Contributor

weiquu commented Jul 24, 2023

Hi @Rajdeep1311, don't forget to incorporate @jasonqiu212's suggestions above! Also in search.md, it would be good to standardise the language, i.e. "If the above command does not work, you may not have the updated v2 version of Docker. You may want to try this instead:"

@Rajdeep1311
Copy link
Contributor Author

Done !!

Rajdeep1311 and others added 2 commits July 24, 2023 07:13
Co-authored-by: Jason Qiu <jason_qiu@hotmail.com>
Co-authored-by: Jason Qiu <jason_qiu@hotmail.com>
@weiquu weiquu requested a review from jasonqiu212 July 24, 2023 09:28
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

Sorry last few changes! Let's try to standardise how we present the alternative command (:

docs/development.md Outdated Show resolved Hide resolved
docs/search.md Outdated Show resolved Hide resolved
Rajdeep1311 and others added 2 commits July 24, 2023 18:20
Co-authored-by: Wei Qing <48304907+weiquu@users.noreply.github.com>
Co-authored-by: Wei Qing <48304907+weiquu@users.noreply.github.com>
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM!

@weiquu weiquu added the s.FinalReview The PR is ready for final review label Jul 24, 2023
Copy link
Contributor

@jasonqiu212 jasonqiu212 left a comment

Choose a reason for hiding this comment

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

LGTM!

@jasonqiu212 jasonqiu212 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Jul 26, 2023
@jasonqiu212 jasonqiu212 merged commit b6e9e30 into TEAMMATES:master Jul 26, 2023
8 checks passed
@samuelfangjw samuelfangjw added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Aug 21, 2023
@samuelfangjw samuelfangjw added this to the V8.29.0 milestone Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation issue - Docker-compose v2 upgrade
4 participants