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

Update scripts/test_lib.sh to include ./tools modules #17760

Closed
ivanvc opened this issue Apr 9, 2024 · 8 comments · Fixed by #17770
Closed

Update scripts/test_lib.sh to include ./tools modules #17760

ivanvc opened this issue Apr 9, 2024 · 8 comments · Fixed by #17770

Comments

@ivanvc
Copy link
Member

ivanvc commented Apr 9, 2024

What would you like to be added?

As identified in #17751, scripts/test_lib.sh currently doesn't include go modules nested in ./tools.

We need to update module_dirs() to include them.

etcd/scripts/test_lib.sh

Lines 168 to 170 in fbda591

function module_dirs() {
echo "api pkg client/pkg client/internal/v2 client/v3 server etcdutl etcdctl tests ."
}

Why is this needed?

Not having these nested modules in test_lib.sh makes them dodge some of our CI/CD workflows.

@ivanvc
Copy link
Member Author

ivanvc commented Apr 9, 2024

Refer to #17751 (comment)

@ivanvc
Copy link
Member Author

ivanvc commented Apr 9, 2024

@jmhbnz, is this a good first-issue candidate? Or not, as there may be additional work/discussion if the proper way of doing it is just adding the two new tools in the module_dirs().

@jmhbnz
Copy link
Member

jmhbnz commented Apr 9, 2024

@jmhbnz, is this a good first-issue candidate? Or not, as there may be additional work/discussion if the proper way of doing it is just adding the two new tools in the module_dirs().

I think it's pretty straightforward to update the script, added labels. Thanks @ivanvc

@tico88612
Copy link
Contributor

/assign

@jimmy-bro
Copy link

/assign

@ivanvc
Copy link
Member Author

ivanvc commented Apr 15, 2024

Hi @zhenguozhang, Thanks for your interest in working on this task. However, it has already been assigned to @tico88612, who raised PR #17770. We're finalizing an active conversation on that PR to close this issue. At this moment, there are no more tasks required for this issue.

@jimmy-bro
Copy link

Thans for your reply, @ivanvc I've been studying etcd for a while now, and I'm ready to start helping the etcd community by fixing some bugs and iterating on new features. I'll take a look at other issues to see if I can address them.

@ivanvc
Copy link
Member Author

ivanvc commented Apr 16, 2024

@zhenguozhang, no worries. Keep an eye out for issues with the good first issue and help wanted labels that don't have an assignee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment