Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Appknox-Go: Region and Host functionality #52

Merged
merged 8 commits into from
Sep 18, 2024

Conversation

yashviagrawal
Copy link
Contributor

Added a region flag, and now host takes url only, and region takes names only

README.md Outdated Show resolved Hide resolved
helper/clientInitialize.go Outdated Show resolved Hide resolved
cmd/root.go Outdated
RootCmd.PersistentFlags().String("region", "", "Region names, e.g., global, saudi, uae. By default, global is used")
viper.BindPFlag("region", RootCmd.PersistentFlags().Lookup("region"))
viper.BindEnv("region", "APPKNOX_API_REGION")
viper.SetDefault("region", "")
Copy link
Member

Choose a reason for hiding this comment

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

The default is suppose to be global. Can we add it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, added.

Comment on lines 52 to 71
if host == "" {
if region != "" {
// Check if region exists in the mappings
if mappedHost, exists := hostMappings[region]; exists {
host = mappedHost
} else {
// Invalid region, throw error and show available regions
availableRegions := make([]string, 0, len(hostMappings))
for key := range hostMappings {
availableRegions = append(availableRegions, key)
}
fmt.Printf("Invalid region name: %s. Available regions: %s\n", region, strings.Join(availableRegions, ", "))
os.Exit(1)
}
} else {
// If neither host nor region are provided, default to the global host
host = hostMappings["global"]
}
} else {
// If both region and host are provided, prioritize host and ignore region
Copy link
Member

Choose a reason for hiding this comment

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

can we move this logic to a separate function along with tests? There is too much nested if here without tests?

the function should probably accept the host env and region env and return (baseurl, error) with similar logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, moved the logic to a separate function.

Comment on lines 36 to 40
func getAPIHostMappings() map[string]string {
// Instead of using an environment variable, call the new function
return GetHostMappings()
}

Copy link
Member

Choose a reason for hiding this comment

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

Is the function needed? GetHostMappings doing the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the function is redundant, fixed that.

Copy link

@cosmosgenius cosmosgenius merged commit 31303db into appknox:develop Sep 18, 2024
2 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants