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

feat: filter changes using go list output #17397

Merged
merged 4 commits into from
Apr 16, 2020

Conversation

kuisathaverat
Copy link
Contributor

What does this PR do?

It extracts the PATH dependencies from the list of Go vendor modules

Why is it important?

This approach is automatic when you add new dependencies, also, it is more accurate filtering better the changes.

Related issues

Relates #16953

@kuisathaverat kuisathaverat self-assigned this Apr 1, 2020
@kuisathaverat kuisathaverat changed the title [WIP] feat: get vendor change patterns feat: get vendor change patterns Apr 6, 2020
@kuisathaverat kuisathaverat changed the title feat: get vendor change patterns feat: filter changes using go list output Apr 6, 2020
@kuisathaverat kuisathaverat requested review from a team, urso, andrewkroh and jsoriano April 6, 2020 17:03
Jenkinsfile Show resolved Hide resolved
Jenkinsfile Show resolved Hide resolved
Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

/LGTM with some NITs

@kuisathaverat
Copy link
Contributor Author

@urso @jsoriano @andrewkroh
The k8s test is only triggered if you change a file in deploy/kubernetes/ for me looks wrong, it means that your code changes will not be tested on k8s. I'd said that merges to master should trigger those test also.

@jsoriano
Copy link
Member

jsoriano commented Apr 7, 2020

@urso @jsoriano @andrewkroh
The k8s test is only triggered if you change a file in deploy/kubernetes/ for me looks wrong, it means that your code changes will not be tested on k8s. I'd said that merges to master should trigger those test also.

Agree, changes in other places can also affect deployments in kubernetes.

@kuisathaverat kuisathaverat merged commit 98255f8 into elastic:master Apr 16, 2020
@kuisathaverat kuisathaverat deleted the vendor branch April 16, 2020 08:28
kuisathaverat added a commit to kuisathaverat/beats that referenced this pull request Apr 16, 2020
* feat: filter changes using go list output

* fix: auditbeat trigger
kuisathaverat added a commit to kuisathaverat/beats that referenced this pull request Apr 16, 2020
* feat: filter changes using go list output

* fix: auditbeat trigger
@urso
Copy link

urso commented Apr 16, 2020

I see I'm a little late to the discussion, but I wonder how effective this change will be.

Can we document what exactly getVendorPatterns will return? Just from reading the code it is far from obvious. Does it contain the package names or the vendor paths?

Given that we always add the regular expressions ^libbeat/.* and ^vendor/.* I wonder what the exact effect of goVendorPatterns will be.
If getVendorPatterns creates regular expressions, would it make sense to anchor those with ^?

Running the provided script locally I get paths like this:

github.com/gofrs/uuid/.*
github.com/gogo/protobuf/proto/.*
github.com/gogo/protobuf/sortkeys/.*
github.com/golang/protobuf/proto/.*
github.com/golang/protobuf/ptypes/.*
github.com/golang/protobuf/ptypes/any/.*
github.com/golang/protobuf/ptypes/duration/.*
github.com/golang/protobuf/ptypes/timestamp/.*
github.com/golang/snappy/.*

These are clearly dependencies that should be in vendor. But the path is incorrect.

Only a small set of packages are prefixed with vendor:

vendor/golang.org/x/crypto/chacha20/.*
vendor/golang.org/x/crypto/chacha20poly1305/.*
vendor/golang.org/x/crypto/cryptobyte/.*
vendor/golang.org/x/crypto/cryptobyte/asn1/.*
vendor/golang.org/x/crypto/curve25519/.*
vendor/golang.org/x/crypto/hkdf/.*
vendor/golang.org/x/crypto/internal/subtle/.*
vendor/golang.org/x/crypto/poly1305/.*
vendor/golang.org/x/net/dns/dnsmessage/.*
vendor/golang.org/x/net/http/httpguts/.*
vendor/golang.org/x/net/http/httpproxy/.*
vendor/golang.org/x/net/http2/hpack/.*
vendor/golang.org/x/net/idna/.*
vendor/golang.org/x/sys/cpu/.*
vendor/golang.org/x/text/secure/bidirule/.*
vendor/golang.org/x/text/transform/.*
vendor/golang.org/x/text/unicode/bidi/.*
vendor/golang.org/x/text/unicode/norm/.*

No idea why these packages are prefixed with vendor, but the output actually includes these packages twice:

golang.org/x/text/transform/.*
golang.org/x/text/unicode/bidi/.*
golang.org/x/text/unicode/norm/.*
vendor/golang.org/x/text/transform/.*
vendor/golang.org/x/text/unicode/bidi/.*
vendor/golang.org/x/text/unicode/norm/.*

@kuisathaverat
Copy link
Contributor Author

Can we document what exactly getVendorPatterns will return? Just from reading the code it is far from obvious. Does it contain the package names or the vendor paths?

It returns the output of go list -mod=vendor adding .* at the end of each line. It also removes the stringgit.luolix.top/elastic/beats/v7 to make the beats paths match.

These are clearly dependencies that should be in vendor. But the path is incorrect.

because not all packages have vendor on the beginning the regexp is generated without '^'

No idea why these packages are prefixed with vendor, but the output actually includes these packages twice:

we can filter them in a follow-up

kuisathaverat added a commit that referenced this pull request Apr 20, 2020
* feat: filter changes using go list output

* fix: auditbeat trigger
jsoriano pushed a commit to jsoriano/beats that referenced this pull request Apr 28, 2020
* feat: filter changes using go list output

* fix: auditbeat trigger
jsoriano added a commit that referenced this pull request Apr 28, 2020
Backport some features added to Jenkinsfile to 7.x branch:
* Dry run option.
* Docker login.
* Git config for generator tests.
* Filter changes using go list.

These are the cherry-picked changes:
* fix: login into the docker registry (#17620)
* feat: filter changes using go list output (#17397)
* fix: disable workaround on macos (#17750)
* ci: set git user configuration if it is not set (#17782)
* fix: mount Docker credentials (#17798)
* Review dependency patterns collection in Jenkins (#18004)

Co-authored-by: Ivan Fernandez Calvo <kuisathaverat@users.noreply.github.com>
Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
)

* feat: filter changes using go list output

* fix: auditbeat trigger
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants