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

rgw: check zone_id or zone_name not empty for zone get sub-command #22957

Closed
wants to merge 1 commit into from

Conversation

david-z
Copy link
Member

@david-z david-z commented Jul 10, 2018

@david-z
Copy link
Member Author

david-z commented Jul 11, 2018

@oritwas Pls help to review this PR, thanks.

@tianshan
Copy link
Contributor

if zone_id & zone_name are both empty, will get the default zone, so I think it's not necessary.

@david-z
Copy link
Member Author

david-z commented Jul 13, 2018

@tianshan Pls check the tracker, you can see if run radosgw-admin zone get, it will return default zone info, but this doesn't make sense because no zone id or zone name is even specified. And if run radosgw-admin zone get XXX, it will return the default zone info. If we don't check both options and return error msg, it will bring much trouble to tell whether this zone info is correct for XXX, especially for cluster operator.

And all the other radosgw-admin zone sub-commands checked both options, I think zone get should do the same.

@tianshan
Copy link
Contributor

@david-z I think it's another issue about zone get XXX that didn't parse the extra param.
I'm agree that make the cmd more certain is good point, meanwhile it's fine return default zone info for me if no zone id or name provided.

Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

agree with @tianshan, this would be a regression that breaks 'radosgw-admin zone get' to read the default zone

@david-z
Copy link
Member Author

david-z commented Jul 16, 2018

@cbodley @tianshan OK, I understand. If this breaks regression test, can we at least output an warning msg to indicate the default zone is returned? Will the warning msg break the regression test?

Thanks.

@david-z
Copy link
Member Author

david-z commented Jul 18, 2018

@cbodley @tianshan

Hi Guys,

Could you pls take a look at my last comment? Or shall we continue following this issue any way?

@cbodley
Copy link
Contributor

cbodley commented Jul 19, 2018

hi @david-z, my concern is not that a warning will break tests. it's more that the 'default zone' is itself a feature - it's controlled with the zone default command and --default option to zone create/modify, and means that zone is used by default when no --rgw-zone is provided - this applies globally to all radosgw/radosgw-admin commands.

it just doesn't seem right to warn people that make use of this feature. i would rather find a way to improve our documentation to eliminate this kind of confusion

@david-z
Copy link
Member Author

david-z commented Jul 23, 2018

Thanks for the explanation. I will close this PR.

@david-z david-z closed this Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants