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

Fix antctl mc deploy command usage #6287

Merged
merged 3 commits into from
May 11, 2024

Conversation

roopeshsn
Copy link
Contributor

This patch ensures that v is optional in the version tag and improves the error message if the specified version tag is not found.

If someone provide an invalid tag the error message would be,

$ ./bin/antctl mc  deploy leadercluster -n antrea-multicluster --antrea-version 2.15.0
Error: specified version tag is not found

This PR addresses the issue #6151

This patch ensures that v is optional in version tag and improved error message if specified version tag is not found.

Signed-off-by: Roopesh Saravanan <roopeshsaravanan.dev@gmail.com>
@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label May 6, 2024
@roopeshsn
Copy link
Contributor Author

Thanks, @luolanzone and @tnqn for the review! After the review changes the new test cases for this patch are failing because the check is present in the deploy function. Can I move the check,

if version != "latest" && !strings.HasPrefix(version, "v") {
    version = fmt.Sprintf("v%s", version)
} 

inside generateManifests?

@tnqn
Copy link
Member

tnqn commented May 7, 2024

After the review changes the new test cases for this patch are failing because the check is present in the deploy function. Can I move the check inside generateManifests?

Yes, actually it's better to move the check there to deal with "version" in a single place. But I don't think it's caused by the review changes, the test failed before the review changes are applied.

Signed-off-by: Roopesh Saravanan <roopeshsaravanan.dev@gmail.com>
@luolanzone
Copy link
Contributor

Hi @roopeshsn Could you check the failure of golanglint result here
It shows pkg/antctl/raw/multicluster/deploy/deploy_helper.go:59: File is not gofmt-ed with -s(gofmt), please do the format and update the PR. Thanks.

Signed-off-by: Roopesh Saravanan <roopeshsaravanan.dev@gmail.com>
@roopeshsn
Copy link
Contributor Author

Hi @roopeshsn Could you check the failure of golanglint result here It shows pkg/antctl/raw/multicluster/deploy/deploy_helper.go:59: File is not gofmt-ed with -s(gofmt), please do the format and update the PR. Thanks.

Updated the PR with formatting.

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented May 11, 2024

/skip-all
/test-multicluster-e2e

@tnqn tnqn merged commit 4ec2bdd into antrea-io:main May 11, 2024
54 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants