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

Remove s3_access As Required From update_account_s3_access #7267

Closed
wants to merge 1 commit into from

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Apr 17, 2023

Explain the changes

  1. Remove s3_access as required from update_account_s3_access. The use of s3_access was historical: before the UI was deprecated we had two types of accounts:
    (1) account access S3
    (2) account UI - account UI was for using the UI and it didn't have s3_access.
    Today all accounts have s3_access.

Issues: Fixed #7204

  1. In the opened issue this is a fix to the comment.

Testing Instructions:

  1. none
  • Doc added/updated
  • Tests added

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
liranmauda
liranmauda previously approved these changes Apr 17, 2023
Copy link
Contributor

@liranmauda liranmauda left a comment

Choose a reason for hiding this comment

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

LGTM

@liranmauda liranmauda dismissed their stale review April 17, 2023 06:47

rethinking...

Copy link
Contributor

@liranmauda liranmauda left a comment

Choose a reason for hiding this comment

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

Based on the explanation, It all accounts should have s3_access then:

  1. In the create_account we should remove the if:
    if (req.rpc_params.s3_access) {
  2. Same goes for the update_account_s3_access:
    if (req.rpc_params.s3_access) {
    and:
    if (req.rpc_params.s3_access) {
    and the else of those if should be removed.
  3. Same goes for the validate_create_account_params:
    if (req.rpc_params.s3_access) {

Another point is that if we don't need it, we should probably remove the require form create_account and in the account_api.
Moreover, we should probably remove it all completely from the api, create an upgrade script, and fix the Operator calls for those.

@nimrod-becker
Copy link
Contributor

I agree with Liran's comment

@shirady
Copy link
Contributor Author

shirady commented Apr 19, 2023

@liranmauda, thank you as discussed with @dannyzaken I'll close this PR for now.

@shirady shirady closed this Apr 19, 2023
@shirady shirady deleted the s3-access-not-required branch July 17, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the Option to Define Default Backing Store by the User Explicitly
3 participants