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

Vendor generated file #1464

Merged
merged 2 commits into from
May 1, 2017
Merged

Vendor generated file #1464

merged 2 commits into from
May 1, 2017

Conversation

vdemeester
Copy link
Contributor

The main reason is : allows compilation (and go test -v, …) on non-unix systems (like windows).
We already vendor dependencies so… 👼

Signed-off-by: Vincent Demeester [email protected]

@timoreimann
Copy link
Contributor

When do developers need to re-run the auto-generation step?

The docs should probably be updated in this regard.

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

@vdemeester Could you modify CONTRIBUTING.md accordingly ?

@vdemeester
Copy link
Contributor Author

@emilevauge yes I can 👼 I also want to add a check (like validate glide) for it (i.e. if a template changes in the commit/diff, validates that the autogen file is correct). But first I need to update validate-gofmt 😂

@timoreimann
Copy link
Contributor

Good point about the extra validation script we need. 👍

@vdemeester
Copy link
Contributor Author

Updated with the validate files 👼

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Left mostly questions.

@@ -0,0 +1,30 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

/usr/bin/env bash is preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep I know, all other scripts are /bin/bash so.. I did a mimic and was about to do a PR to switch to /usr/bin/env bash

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather see this being updated either for validate-autogen or all scripts right on this PR. Otherwise, chances are the script will not work until you submitted that follow-up PR. (For instance, on my Mac /bin/bash points at the ancient bash 3 version that doesn't work with many features.)

@@ -3,7 +3,7 @@
source "$(dirname "$BASH_SOURCE")/.validate"

IFS=$'\n'
files=( $(validate_diff --diff-filter=ACMR --name-only -- '*.go' | grep -v '^\(integration/\)\?vendor/' || true) )
files=( $(validate_diff --diff-filter=ACMR --name-only -- '*.go' | grep -v '^\(integration/\)\?vendor/\|autogen' || true) )
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you verify the pipe operator works (that is, it really ignores the autogen file while not breaking the vendor things)?

Last time I tried, I had quite some issues, which is why I'm asking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this now match an autogen file located anywhere in our code base? Is that what we want? Do we need to be more / less specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes for the first questio.
to test for the 2nd 👼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's ok for the second (i.e. provider/autogen.go for ex isn't exclude by this and this will be verified)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -72,6 +72,10 @@ $ gox "linux darwin" "386 amd64 arm" \
# run other commands like tests
```

### Updating the templates

If you happen to update the provider templates (in `/templates`), you need to run `go generate` to update the `autogen` package.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another comment in the file mentions that the generation step is required for the web UI. If that's true, we should mention it there as well and possibly update the other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look 👼

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Two points left, half-suggestion/half-requirement. :-)

@@ -61,7 +61,8 @@ Here's a full example:
$ ./script/glide.sh get github.com/foo/bar
# install another dependency, this time for the integration tests
$ ( cd integration && ../script/glide.sh get github.com/baz/quuz )
# generate (Only required to integrate other components such as web dashboard)
# generate
# (Only required to integrate other components such as web dashboard and if you change something there — web, templates)
Copy link
Contributor

Choose a reason for hiding this comment

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

if you change something there [being the dashboard]

The templates aren't really related to the dashboard, are they?

How about rephrasing it like this:

# generate
# (required to merge non-code components into the final binary, such as the web dashboard and provider Go templates)

@@ -0,0 +1,30 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather see this being updated either for validate-autogen or all scripts right on this PR. Otherwise, chances are the script will not work until you submitted that follow-up PR. (For instance, on my Mac /bin/bash points at the ancient bash 3 version that doesn't work with many features.)

@@ -3,7 +3,7 @@
source "$(dirname "$BASH_SOURCE")/.validate"

IFS=$'\n'
files=( $(validate_diff --diff-filter=ACMR --name-only -- '*.go' | grep -v '^\(integration/\)\?vendor/' || true) )
files=( $(validate_diff --diff-filter=ACMR --name-only -- '*.go' | grep -v '^\(integration/\)\?vendor/\|autogen' || true) )
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM. 👏

@vdemeester
Copy link
Contributor Author

ping @containous/traefik 👼 (I'll rebase once I have enough LGTM…)

@ldez ldez added this to the 1.3 milestone Apr 25, 2017
@ldez
Copy link
Contributor

ldez commented Apr 28, 2017

@vdemeester it's time to rebase! 😃

SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"; export SCRIPTDIR
source "${SCRIPTDIR}/.validate"

# Iterate over all directories containing vendor folders.
Copy link
Member

Choose a reason for hiding this comment

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

s/vendor/templates ?

Signed-off-by: Vincent Demeester <[email protected]>
… instead of /bin/bash, to work better on more platforms.

Signed-off-by: Vincent Demeester <[email protected]>
@emilevauge emilevauge merged commit a0d6594 into traefik:master May 1, 2017
@vdemeester vdemeester deleted the vendor-autogen branch May 1, 2017 17:16
@emilevauge
Copy link
Member

@vdemeester, As discussing with @ldez, it seems the validation script is incomplete and lacks webui checks (in go generate: go-bindata -pkg autogen -o autogen/gen.go ./static/... ./templates/...).
But adding a webui check in validation will be a lot slower, building webui takes some time.
WDYT? revert?

ldez added a commit that referenced this pull request May 3, 2017
@ldez ldez added kind/enhancement a new or improved feature. and removed status/3-needs-merge labels May 19, 2017
@ldez ldez mentioned this pull request Nov 20, 2017
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants