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

chore: cleanup #15514 (Rewrite etcd scripts in strict mode) #15672

Merged
merged 5 commits into from
Apr 13, 2023

Conversation

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

LGTM. Great cleanup!

Some background, those scripts were already broken when I joined the project. We left them so someone maybe will want to fix them, however no-one wanted to use manual tests and robustness tests automated majority of the tests done here.

@fuweid fuweid marked this pull request as ready for review April 11, 2023 08:28
@fuweid
Copy link
Member Author

fuweid commented Apr 11, 2023

Rebased! Mark it ready to review.

@fuweid
Copy link
Member Author

fuweid commented Apr 12, 2023

ping @serathius @ahrtr ~

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @fuweid

@chaochn47
Copy link
Member

chaochn47 commented Apr 12, 2023

Some background, those scripts were already broken when I joined the project. We left them so someone maybe will want to fix them, however no-one wanted to use manual tests

I don't see the original manual test commit was justified with any test results or associated with a PR with discussion a2256a6.

I agree we should delete not maintained manual test scripts and in favor of automated tests that are verified continuously.

echo "cfssl is not installed"
echo "use: go install -mod mod github.com/cloudflare/cfssl/cmd/cfssl"
Copy link
Member

@ahrtr ahrtr Apr 12, 2023

Choose a reason for hiding this comment

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

FYI. -mod=mod tells the go command to ignore the vendor directory and to [automatically update](https://go.dev/ref/mod#go-mod-file-updates) go.mod, for example, when an imported package is not provided by any known module.. I don't think it should automatically update the go.mod file. See https://go.dev/ref/mod#build-commands.

This comment applies to a couple of places below.

Suggested change
echo "use: go install -mod mod github.com/cloudflare/cfssl/cmd/cfssl"
echo "use: go install github.com/cloudflare/cfssl/cmd/cfssl"

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. It will dirty the go.mod if the user runs the command in the repo. I updated with your suggestion. Please take a look. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Please consider to install a specific version of tool (e.g. cfssl in this case).

Usually we specify the version in tools/mod:

  1. import the package in tools/mod/tools.go;
  2. specify the version in tools/mod/go.mod;
  3. Execute cd tools/mod; go install github.com/cloudflare/cfssl/cmd/cfssl

This can be resolved in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! That makes sense. Will file other pr later.

@fuweid
Copy link
Member Author

fuweid commented Apr 13, 2023

cluster_test.go:326: expect error to contain can only promote a learner member, got etcdserver: request timed out

TestMemberPromoteMemberNotLearner is flaky as known issue. Need a maintainer to retry~ Thanks

@fuweid fuweid closed this Apr 13, 2023
@fuweid fuweid reopened this Apr 13, 2023
Copy link
Member

@ahrtr ahrtr 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 a minor comment, which can be resolved separately.

Thank you @fuweid

echo "cfssl is not installed"
echo "use: go install -mod mod github.com/cloudflare/cfssl/cmd/cfssl"
Copy link
Member

Choose a reason for hiding this comment

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

It still has "-mod mod"

Suggested change
echo "use: go install -mod mod github.com/cloudflare/cfssl/cmd/cfssl"
echo "use: go install github.com/cloudflare/cfssl/cmd/cfssl"

REF: etcd-io#15514

Signed-off-by: Wei Fu <fuweid89@gmail.com>
REF: etcd-io#15514

Signed-off-by: Wei Fu <fuweid89@gmail.com>
REF: etcd-io#15514

Signed-off-by: Wei Fu <fuweid89@gmail.com>
REF: etcd-io#15514

Signed-off-by: Wei Fu <fuweid89@gmail.com>
Signed-off-by: Wei Fu <fuweid89@gmail.com>
echo "cfssl is not installed"
echo 'use: bash -c "cd ../../../tools/mod; go install github.com/cloudflare/cfssl/cmd/cfssl"'
Copy link
Member

Choose a reason for hiding this comment

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

This comment applies to some places below as well.

Note that this script may be executed at any directory, so set the absolute directory is better.

Suggested change
echo 'use: bash -c "cd ../../../tools/mod; go install github.com/cloudflare/cfssl/cmd/cfssl"'
echo 'use: bash -c "cd ${ETCD_ROOT_DIR}/tools/mod; go install github.com/cloudflare/cfssl/cmd/cfssl"'

Copy link
Member Author

Choose a reason for hiding this comment

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

This script is only allowed to run in current dir.

if ! [[ "$0" =~ "./gencerts.sh" ]]; then
  echo "must be run from 'fixtures'"
  exit 255
fi

So I was thinking that the user can copy-parse the echo string to install.
If we use ETCD_ROOT_DIR, the user needs to get the root dir path first.

I can add one line to resolve ETCD_ROOT_DIR first.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then let's keep it as it is for now.

@ahrtr ahrtr merged commit 22f3e50 into etcd-io:main Apr 13, 2023
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.

5 participants