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.

Sign up for GitHub

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

feat(region): more debug logs #1007

Merged
merged 4 commits into from
Sep 22, 2023
Merged

feat(region): more debug logs #1007

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ddd329
feat(region): more debug logs
EtienneM Sep 18, 2023
0b1dcd8
refactor(region): more idiomatic Go
EtienneM Sep 18, 2023
1d9d0aa
docs: missing changelog entry
EtienneM Sep 18, 2023
531c972
fix(region): missing err checks
EtienneM Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* feat: add database maintenance listing with the new `database-maintenance-list` command ([PR#982](https://github.com/Scalingo/cli/pull/982))
* feat(addons): add maintenance windows manipulation with the new `addon-config` command ([PR#955](https://github.com/Scalingo/cli/pull/955))
* feat(install.sh): verify the archive checksum ([PR#988](https://github.com/Scalingo/cli/pull/988))
* feat(region): more debug logs ([PR#1007](https://github.com/Scalingo/cli/pull/1008))

### 1.29.1

Expand Down
22 changes: 15 additions & 7 deletions config/region.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,12 @@ type GetRegionOpts struct {
}

func EnsureRegionsCache(ctx context.Context, c Config, opts GetRegionOpts) (RegionsCache, error) {
debug.Println("[Regions] Ensure cache is filled")
var regionsCache RegionsCache
fd, err := os.Open(c.RegionsCachePath)
if err == nil {
if err != nil {
debug.Printf("[Regions] Fail to open the cache: %v\n", err)
} else {
Comment on lines +55 to +57
Copy link
Member Author

Choose a reason for hiding this comment

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

We are more used to check err != nil in Go. So I'm refactoring to use this pattern here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why don't we use defer fd.Close()?
It seems more secure IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. Maybe it's useless in this case to wait longer before closing the file descriptor: we decode its content then immediately close it.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM Then 👌

json.NewDecoder(fd).Decode(&regionsCache)
fd.Close()
}
Expand All @@ -63,6 +66,7 @@ func EnsureRegionsCache(ctx context.Context, c Config, opts GetRegionOpts) (Regi

var client *scalingo.Client
if opts.SkipAuth {
debug.Println("[Regions] Create an unauthenticated client to the authentication service")
client, err = ScalingoUnauthenticatedAuthClient(ctx)
if err != nil {
return RegionsCache{}, errgo.Notef(err, "fail to create an unauthenticated client")
Expand All @@ -77,13 +81,14 @@ func EnsureRegionsCache(ctx context.Context, c Config, opts GetRegionOpts) (Regi
}
}

debug.Println("[Regions] Get the list of regions to fill the cache")
debug.Println("[Regions] Create an authenticated client to the authentication service using the token")
client, err = ScalingoAuthClientFromToken(ctx, token.Token)
if err != nil {
return RegionsCache{}, errgo.Notef(err, "fail to create an authenticated Scalingo client using the API token")
}
}

debug.Println("[Regions] Get the list of regions to fill the cache")
regions, err := client.RegionsList(ctx)
if err != nil {
return RegionsCache{}, errgo.Notef(err, "fail to list available regions")
Expand All @@ -93,18 +98,20 @@ func EnsureRegionsCache(ctx context.Context, c Config, opts GetRegionOpts) (Regi
regionsCache.ExpireAt = time.Now().Add(10 * time.Minute)

if opts.SkipAuth {
debug.Println("[Regions] Do not save the cache")
// If we skipped the authentication the region cache should not be saved since it will not contain regions that are not publicly available (like osc-secnum-fr1)
return regionsCache, nil
}

debug.Println("[Regions] Save the cache")
fd, err = os.OpenFile(c.RegionsCachePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0750)
if err == nil {
json.NewEncoder(fd).Encode(regionsCache)
fd.Close()
} else {
debug.Printf("[Regions] Failed to save the regions cache: %v\n", err)
if err != nil {
debug.Printf("[Regions] Failed to save the cache: %v\n", err)
return regionsCache, nil
}
Comment on lines +111 to 114
Copy link
Member Author

Choose a reason for hiding this comment

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

We are more used to check err != nil in Go. So I'm refactoring to use this pattern here.


json.NewEncoder(fd).Encode(regionsCache)
fd.Close()
return regionsCache, nil
}

Expand All @@ -119,6 +126,7 @@ func GetRegion(ctx context.Context, c Config, name string, opts GetRegionOpts) (

for _, region := range regionsCache.Regions {
if region.Name == name {
debug.Println("[Regions] Found the region in the cache")
return region, nil
}
}
Expand Down