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

etcd: Replace yaml dependency github.com/ghodss/yaml with sigs.k8s.io/yaml #10687

Conversation

rohitsardesai83
Copy link
Contributor

To remove the dependency on ghodss/yaml. Replaced this dependency with sigs.k8s.io/yaml.
This wil help to remove the ghodss/yaml dependency from main kubernetes repository.

xref: kubernetes/kubernetes#77024.

@neolit123
Copy link

@rohitsardesai83
i'm pretty sure the library should be vendored and the old one removed.
see:
https://github.com/etcd-io/etcd/tree/master/vendor/github.com/ghodss/yaml
https://travis-ci.com/etcd-io/etcd/jobs/196125065

mbed/config.go:30:2: cannot find package "sigs.k8s.io/yaml" in any of:
	/go/src/go.etcd.io/etcd/vendor/sigs.k8s.io/yaml (vendor tree)
	/usr/local/go/src/sigs.k8s.io/yaml (from $GOROOT)
	/go/src/sigs.k8s.io/yaml (from $GOPATH)
The command "case "${TARGET}" in

@jingyih
Copy link
Contributor

jingyih commented Apr 29, 2019

@rohitsardesai83 Can you try if scripts/updatedep.sh fix the vendor?

@rohitsardesai83 rohitsardesai83 force-pushed the replace_ghodss_yaml_with_sigsk8sio_yaml branch from ad3e4f5 to ea5808a Compare April 29, 2019 05:08
@codecov-io
Copy link

codecov-io commented Apr 29, 2019

Codecov Report

Merging #10687 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10687      +/-   ##
==========================================
- Coverage   71.62%   71.58%   -0.05%     
==========================================
  Files         394      394              
  Lines       36659    36659              
==========================================
- Hits        26258    26242      -16     
- Misses       8569     8577       +8     
- Partials     1832     1840       +8
Impacted Files Coverage Δ
clientv3/yaml/config.go 86.66% <ø> (ø) ⬆️
etcdmain/config.go 82.48% <ø> (ø) ⬆️
embed/config.go 56.37% <ø> (ø) ⬆️
proxy/grpcproxy/register.go 80.55% <0%> (-13.89%) ⬇️
proxy/grpcproxy/watcher.go 89.79% <0%> (-4.09%) ⬇️
etcdserver/util.go 95% <0%> (-3.75%) ⬇️
proxy/grpcproxy/watch.go 89.75% <0%> (-3.02%) ⬇️
etcdserver/api/v3rpc/watch.go 80.06% <0%> (-2.29%) ⬇️
raft/progress.go 94.23% <0%> (-1.93%) ⬇️
clientv3/balancer/grpc1.7-health.go 58.13% <0%> (-1.75%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efcc108...d236577. Read the comment docs.

@rohitsardesai83
Copy link
Contributor Author

@rohitsardesai83 Can you try if scripts/updatedep.sh fix the vendor?

@jingyih , yes thanks for the help , I did fix the vendor !

@rohitsardesai83
Copy link
Contributor Author

@rohitsardesai83
i'm pretty sure the library should be vendored and the old one removed.
see:
https://github.com/etcd-io/etcd/tree/master/vendor/github.com/ghodss/yaml
https://travis-ci.com/etcd-io/etcd/jobs/196125065

mbed/config.go:30:2: cannot find package "sigs.k8s.io/yaml" in any of:
	/go/src/go.etcd.io/etcd/vendor/sigs.k8s.io/yaml (vendor tree)
	/usr/local/go/src/sigs.k8s.io/yaml (from $GOROOT)
	/go/src/sigs.k8s.io/yaml (from $GOPATH)
The command "case "${TARGET}" in

@neolit123 , thanks for pointing that out !

@rohitsardesai83 rohitsardesai83 force-pushed the replace_ghodss_yaml_with_sigsk8sio_yaml branch from ea5808a to 6dc162a Compare April 29, 2019 14:41
@rohitsardesai83
Copy link
Contributor Author

@neolit123 , @jingyih , I have updated bom json , still the CI job is failing : https://travis-ci.com/etcd-io/etcd/jobs/196350107.

Earlier semaphoreci passed , but now after i pushed the bom file update , it also failed.
Not sure if I am missing something ?

@jingyih
Copy link
Contributor

jingyih commented Apr 29, 2019

@rohitsardesai83 Error says "vendored licenses do not match given bill of materials". Does scripts/updatebom.sh help?

@rohitsardesai83
Copy link
Contributor Author

@jingyih, yes I did run script and pushed the updated bom

@spzala
Copy link
Member

spzala commented Apr 30, 2019

@neolit123 , @jingyih , I have updated bom json , still the CI job is failing : https://travis-ci.com/etcd-io/etcd/jobs/196350107.

Earlier semaphoreci passed , but now after i pushed the bom file update , it also failed.
Not sure if I am missing something ?
@rohitsardesai83 semaphoreci is little flaky currently and it's known so don't worry about it unless there is a real error which isn't case right now.

@jingyih
Copy link
Contributor

jingyih commented Apr 30, 2019

Can you try again? I fetched your changes and rerun updatebom.sh locally. It was able to fix the bom error.

@rohitsardesai83 rohitsardesai83 force-pushed the replace_ghodss_yaml_with_sigsk8sio_yaml branch from 6dc162a to d236577 Compare April 30, 2019 06:10
@rohitsardesai83
Copy link
Contributor Author

@jingyih , the bom is correctly updated . For some reason , another test failed

=== RUN   TestBalancerUnderBlackholeKeepAliveWatch
--- FAIL: TestBalancerUnderBlackholeKeepAliveWatch (5.76s)
    black_hole_test.go:83: took too long to receive watch events

Failing job: https://travis-ci.com/etcd-io/etcd/jobs/196534632

@rohitsardesai83
Copy link
Contributor Author

Can you try again? I fetched your changes and rerun updatebom.sh locally. It was able to fix the bom error.

Thanks , I guess it was an environment issue, retried with a fresh env and the bom got correctly updated. The bom error is resolved now.

Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

lgtm.
Failed test is flaky / unrelated.

@rohitsardesai83
Copy link
Contributor Author

@neolit123 , @jingyih , I have updated bom json , still the CI job is failing : https://travis-ci.com/etcd-io/etcd/jobs/196350107.
Earlier semaphoreci passed , but now after i pushed the bom file update , it also failed.
Not sure if I am missing something ?
@rohitsardesai83 semaphoreci is little flaky currently and it's known so don't worry about it unless there is a real error which isn't case right now.

Thanks @spzala for the info !

@rohitsardesai83
Copy link
Contributor Author

@jingyih , please help in rerunning the tests , the CI build abruptly stopped with some error.

@rohitsardesai83
Copy link
Contributor Author

@jingyih , @neolit123 , all checks have passed. Thank you !

"go.etcd.io/etcd/embed"
"go.etcd.io/etcd/pkg/flags"
"go.etcd.io/etcd/pkg/types"
"go.etcd.io/etcd/version"

"github.com/ghodss/yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

replace this line with sigs.k8s.io/yaml instead of creating a new import block at line 29

Choose a reason for hiding this comment

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

placed this at the bottom instead of simply replacing the existing ghodss dependency as the 3rd party imports should be sorted alphabetically.

@@ -36,7 +38,6 @@ import (
"go.etcd.io/etcd/pkg/transport"
"go.etcd.io/etcd/pkg/types"

"github.com/ghodss/yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

replace this line with sigs.k8s.io/yaml instead of creating a new import block at line 30

Choose a reason for hiding this comment

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

placed this at the bottom instead of simply replacing the existing ghodss dependency as the 3rd party imports should be sorted alphabetically.

@rohitsardesai83 rohitsardesai83 force-pushed the replace_ghodss_yaml_with_sigsk8sio_yaml branch 3 times, most recently from 6a8193d to e83f311 Compare May 2, 2019 07:00
@rohitsardesai50
Copy link

thanks @xiang90 for your review comments , I have resolved the comments and updated the PR. Please take a look.

@rohitsardesai83 rohitsardesai83 force-pushed the replace_ghodss_yaml_with_sigsk8sio_yaml branch from e83f311 to 9686ec3 Compare May 2, 2019 07:03
To remove the dependency on ghodss/yaml. Replaced this dependency with sigs.k8s.io/yaml.
This wil help to remove the ghodss/yaml dependency from main kubernetes repository.

xref: kubernetes/kubernetes#77024
@rohitsardesai83 rohitsardesai83 force-pushed the replace_ghodss_yaml_with_sigsk8sio_yaml branch from 9686ec3 to 42a7ea6 Compare May 2, 2019 07:04
@gyuho gyuho merged commit e9f310a into etcd-io:master May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants