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

Restructure the documentation #177

Merged
merged 2 commits into from
Aug 6, 2019

Conversation

swapnilgm
Copy link
Contributor

Signed-off-by: Swapnil Mhamane [email protected]

What this PR does / why we need it:
Restructure the documentation, with some minor updates regrading go module for dependency management.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

NONE

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 12, 2019
@swapnilgm swapnilgm force-pushed the restructure-documentation branch from 15ea92f to 384b65e Compare July 12, 2019 12:37
@swapnilgm swapnilgm force-pushed the restructure-documentation branch from 384b65e to 52203f2 Compare July 22, 2019 06:20
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 22, 2019
@swapnilgm
Copy link
Contributor Author

@shreyas-s-rao I updated the PR. PTAL.

Signed-off-by: Swapnil Mhamane <[email protected]>
@swapnilgm swapnilgm force-pushed the restructure-documentation branch from 3695810 to da3aa82 Compare July 30, 2019 10:31
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@swapnilgm Thanks for the excellent and much-required PR! Thanks for the updating the docs as well. I have suggested a couple of changes in the CI scripts as well as a few vocabulary changes for the docs. Please address them. Thanks.

doc/development/testing_and_dependencies.md Show resolved Hide resolved
.ci/unit_test Outdated
export GOBIN="${SOURCE_PATH}/tmp/bin"
export PATH="${GOBIN}:${PATH}"
fi
cd "${SOURCE_PATH}"

# Install Ginkgo (test framework) to be able to execute the tests.
go get -u github.com/onsi/ginkgo/ginkgo
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
go get -u github.com/onsi/ginkgo/ginkgo
GO111MODULE=off go get -u github.com/onsi/ginkgo/ginkgo

.ci/integration_test Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
doc/development/local_setup.md Show resolved Hide resolved
doc/development/testing_and_dependencies.md Outdated Show resolved Hide resolved
doc/development/testing_and_dependencies.md Outdated Show resolved Hide resolved
doc/development/testing_and_dependencies.md Outdated Show resolved Hide resolved
doc/development/testing_and_dependencies.md Outdated Show resolved Hide resolved
@swapnilgm swapnilgm force-pushed the restructure-documentation branch 2 times, most recently from c03f75c to 98158cf Compare August 6, 2019 05:11
@swapnilgm
Copy link
Contributor Author

thanks you for detailed review. I have addressed comments. PTAL.

Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@swapnilgm Thanks for making the requested changes. I've suggested just a few more changes. Please address them.

.ci/integration_test:L72,L48: s/etcbrctl/etcdbrctl/g
.ci/integration_test:L74: Add GO111MODULE=ON to go build command
doc/proposals/design.md:L42,L79: (images/_____) -> (../images/_____)

@@ -27,25 +27,12 @@ VCS="github.com"
ORGANIZATION="gardener"
PROJECT="etcd-backup-restore"
REPOSITORY=${VCS}/${ORGANIZATION}/${PROJECT}
VERSION_FILE="$(readlink -f "${SOURCE_PATH}/VERSION")"
VERSION_FILE="$(readlink -f "${SOURCE_PATH}/VERSION")"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
VERSION_FILE="$(readlink -f "${SOURCE_PATH}/VERSION")"
VERSION_FILE="$(readlink -f "${SOURCE_PATH}/VERSION")"

### Updating dependencies

The `Makefile` contains a rule called `revendor` which performs `go mod vendor` and `go mod tidy`.
* `go mod vendor` resets the main module's vendor directory to include all packages needed to build and test all the main module's packages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `go mod vendor` resets the main module's vendor directory to include all packages needed to build and test all the main module's packages.
* `go mod vendor` resets the main module's vendor directory to include all packages needed to build and test all the main module's packages. It does not include test code for vendored packages.

@swapnilgm swapnilgm force-pushed the restructure-documentation branch from 98158cf to c729979 Compare August 6, 2019 10:36
@swapnilgm
Copy link
Contributor Author

Done

Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

LGTM

@shreyas-s-rao shreyas-s-rao added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 6, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 6, 2019
Co-Authored-By: Shreyas Rao <[email protected]>
Signed-off-by: Swapnil Mhamane <[email protected]>
@swapnilgm swapnilgm force-pushed the restructure-documentation branch from c729979 to f620df1 Compare August 6, 2019 10:41
@shreyas-s-rao shreyas-s-rao added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 6, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 6, 2019
@shreyas-s-rao shreyas-s-rao added area/documentation Documentation related component/etcd-backup-restore ETCD Backup & Restore labels Aug 6, 2019
@shreyas-s-rao shreyas-s-rao added the reviewed/lgtm Has approval for merging label Aug 6, 2019
@shreyas-s-rao shreyas-s-rao merged commit a17381e into gardener:master Aug 6, 2019
@swapnilgm swapnilgm deleted the restructure-documentation branch October 23, 2019 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Documentation related component/etcd-backup-restore ETCD Backup & Restore needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants