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

update description about vendoring in CONTRIBUTING.md #18280

Merged
merged 13 commits into from
Jan 17, 2022
Merged

update description about vendoring in CONTRIBUTING.md #18280

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
dc9a3df
update description about vendoring in CONTRIBUTING.md
a1012112796 Jan 15, 2022
8a6b1d0
Update CONTRIBUTING.md
a1012112796 Jan 15, 2022
620ee47
Merge branch 'main' into remove_vendor_doc
a1012112796 Jan 15, 2022
686e8de
fix and readd test-vendor step
a1012112796 Jan 15, 2022
c91dcb9
Merge branch 'main' into remove_vendor_doc
zeripath Jan 15, 2022
4789e70
remove vendor from .gitattributes @silverwind
a1012112796 Jan 16, 2022
a287e33
Merge branch 'main' into remove_vendor_doc
a1012112796 Jan 16, 2022
74d6f59
Merge branch 'main' into remove_vendor_doc
wxiaoguang Jan 16, 2022
dd0cd45
Merge branch 'main' into remove_vendor_doc
a1012112796 Jan 16, 2022
6aef0e8
simplify go mod check
a1012112796 Jan 16, 2022
18c1abb
Merge branch 'main' into remove_vendor_doc
a1012112796 Jan 17, 2022
d61ada4
Revert "remove vendor from .gitattributes @silverwind"
a1012112796 Jan 17, 2022
3f7b3f5
Merge branch 'main' into remove_vendor_doc
6543 Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
* text=auto eol=lf
/vendor/** -text -eol linguist-vendored
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
/public/vendor/** -text -eol linguist-vendored
/templates/**/*.tmpl linguist-language=Handlebars
/.eslintrc linguist-language=YAML
Expand Down
7 changes: 3 additions & 4 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,12 @@ Here's how to run the test suite:

## Vendoring

We keep a cached copy of dependencies within the `vendor/` directory,
managing updates via [Modules](https://golang.org/cmd/go/#hdr-Module_maintenance).
We manage dependencies via [Go Modules](https://golang.org/cmd/go/#hdr-Module_maintenance), more details: [go mod](https://go.dev/ref/mod).

Pull requests should only include `vendor/` updates if they are part of
Pull requests should only include `go.mod`, `go.sum` updates if they are part of
the same change, be it a bugfix or a feature addition.
Comment on lines +96 to 97
Copy link
Contributor

Choose a reason for hiding this comment

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

I can not quite understand the purpose of this description. Maybe it can be simplified. The Go Modules document already contains all information.

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @lunny @techknowlogick

Suggested change
Pull requests should only include `go.mod`, `go.sum` updates if they are part of
the same change, be it a bugfix or a feature addition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the proposed change is correct and should remain (see below for the rationale).

Copy link
Contributor

Choose a reason for hiding this comment

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

The old text was talking about vendor directory. But now, that is it talking about?

if they are part of the same change: what is the same change ...
be it a bugfix or a feature addition what does the bug fix or feature addition refer to ....


The `vendor/` update needs to be justified as part of the PR description,
The `go.mod`, `go.sum` update needs to be justified as part of the PR description,
and must be verified by the reviewers and/or merger to always reference
an existing upstream commit.
Comment on lines +99 to 101
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use Go Modules, we do not need to check the upstream commit again. This check was required by the committed vendor directory in my mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

The intent of this paragraph is to require a justification whenever a dependency is added or modified. I think the proposed change is correct. Adding a new dependency previously meant modifying vendor/, go.mod and go.sum. Requiring an explanation when vendor/ was modified implied requiring an explanation when go.mod and go.sum was modified. Now that vendor/ is removed, there is a need to rephrase the requirement.


Expand Down
4 changes: 2 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ checks: checks-frontend checks-backend
checks-frontend: lockfile-check svg-check

.PHONY: checks-backend
checks-backend: swagger-check swagger-validate
checks-backend: test-vendor swagger-check swagger-validate

.PHONY: lint
lint: lint-frontend lint-backend
Expand Down Expand Up @@ -371,7 +371,7 @@ vendor:

.PHONY: test-vendor
test-vendor: vendor
@diff=$$(git diff vendor/); \
@diff=$$(git diff go.mod go.sum); \
if [ -n "$$diff" ]; then \
echo "Please run 'make vendor' and commit the result:"; \
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
echo "$${diff}"; \
Expand Down