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

PCD-649 remove region'\''s default value and make it compulsory #399

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

talha3735928559
Copy link

@talha3735928559 talha3735928559 commented Feb 2, 2025

ISSUE(S):

PCD-649

SUMMARY

Region Name defaulted to "RegionOne" before, now no default its a mandatory feild.

ISSUE TYPE

  • Bug fix (non-breaking change which fixes an issue)

IMPACTED FEATURES/COMPONENTS:

cloud-ctl

RELATED ISSUE(S):

DEPENDS ON:

TESTING DONE

Automated

Manual

Inprogress

Reviewers

Summary by Bito

Removed default 'RegionOne' value from configuration system, making region specification mandatory for users. Updated region prompt messaging and documentation to reflect this change. This modification enforces explicit region selection rather than relying on default values.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

Copy link

bito-code-review bot commented Feb 2, 2025

Code Review Agent Run #cc9d18

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 1385b85..1385b85
    • pkg/config/config.go
  • Files skipped - 0
  • Tools
    • Golangci-lint (Linter) - ✖︎ Failed
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by Bito Logo

Copy link

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - Region Configuration Enhancement

config.go - Removed default region value 'RegionOne' and made region specification mandatory

Copy link

@poojaghumre poojaghumre left a comment

Choose a reason for hiding this comment

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

Can you please update the testing section based on this change?

@@ -339,11 +339,11 @@ func ConfigCmdCreateRun(cfg *objects.Config) error {
proxyURL, _ = reader.ReadString('\n')
cfg.ProxyURL = strings.TrimSpace(proxyURL)
}

/* Removing Default Region.

Choose a reason for hiding this comment

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

nit: no need to comment this block

@poojaghumre
Copy link

Putting this on hold as its modifying the base pf9ctl and not just cloud-ctl for PCD multi-region usecase.

@poojaghumre poojaghumre added the hold merging Do not merge this PR label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold merging Do not merge this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants