Closed gopa-noaa closed 7 months ago
My other comment would be that, I DO think we should disable the Python CI for the Go code. So we'd need to change:
To something like this. Note the whitespace is likely off:
name: "CI"
on:
push:
tags:
- "[0-9]+.[0-9]+.[0-9]+"
- "[0-9]+.[0-9]+.[0-9]+-rc[0-9]+"
branches: [main]
+ paths-ignore:
+ - "./meta_update_middleware"
pull_request:
+ paths-ignore:
+ - "./meta_update_middleware"
@ian-noaa thanks for your code improvements suggestions, will incorporate. I agree CI changing to GO specific instead of Python. I also like your suggestion of unit tests integrated into CI, I will add that down the road for middle-ware.
@ian-noaa resolved all conversations except the CI stuff, let me know if this is something you could take care of ...
I like #4
I think you may be right. https://pkg.go.dev/os#ReadFile the example here does not do a close. Looking here https://go.dev/src/os/file.go I see that os.ReadFile calls func ReadFile(name string) ([]byte, error) {
715 f, err := Open(name) 716 if err != nil { 717 return nil, err 718 } 719 defer f.Close()
and it is performaing a close.
On Fri, Jan 19, 2024 at 1:09 PM Gopa @.***> wrote:
@.**** commented on this pull request.
In meta_update_middleware/meta-update.go https://github.com/NOAA-GSL/VxIngest/pull/298#discussion_r1459683695:
}
- defer f.Close()
Since I am using yamlFile, err := os.ReadFile(credentialsFilePath) I don't think we need to Close, since return yamlFile is a byte array
— Reply to this email directly, view it on GitHub https://github.com/NOAA-GSL/VxIngest/pull/298#discussion_r1459683695, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDVQPRPSMQLZKS7YSAVBE3YPLHHFAVCNFSM6AAAAABCA2AQOOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMZTGYYDCNJVGU . You are receiving this because your review was requested.Message ID: @.***>
-- Randy Pierce
Better to just step through it in the debugger to be sure.
On Fri, Jan 19, 2024 at 1:09 PM Gopa @.***> wrote:
@.**** commented on this pull request.
In meta_update_middleware/meta-update.go https://github.com/NOAA-GSL/VxIngest/pull/298#discussion_r1459683695:
}
- defer f.Close()
Since I am using yamlFile, err := os.ReadFile(credentialsFilePath) I don't think we need to Close, since return yamlFile is a byte array
— Reply to this email directly, view it on GitHub https://github.com/NOAA-GSL/VxIngest/pull/298#discussion_r1459683695, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDVQPRPSMQLZKS7YSAVBE3YPLHHFAVCNFSM6AAAAABCA2AQOOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMZTGYYDCNJVGU . You are receiving this because your review was requested.Message ID: @.***>
-- Randy Pierce
I think Gopa's correct. The os.ReadFile()
source does do a defer f.Close()
internally. I'm not sure what doing a defer []byte.Close()
would do in Go.
https://cs.opensource.google/go/go/+/refs/tags/go1.21.6:src/os/file.go;l=710-728
// ReadFile reads the named file and returns the contents.
// A successful call returns err == nil, not err == EOF.
// Because ReadFile reads the whole file, it does not treat an EOF from Read
// as an error to be reported.
func ReadFile(name string) ([]byte, error) {
f, err := Open(name)
if err != nil {
return nil, err
}
defer f.Close()
var size int
if info, err := f.Stat(); err == nil {
size64 := info.Size()
if int64(int(size64)) == size64 {
size = int(size64)
}
}
size++ // one byte for final read at EOF
Regarding CI - sounds good Gopa, I'll push a commit to this PR turning the Python CI off for this Go application.
Package | Line Rate | Branch Rate | Health |
---|---|---|---|
vxingest | 33% | 42% | ❌ |
vxingest.builder_common | 29% | 17% | ❌ |
vxingest.ctc_to_cb | 12% | 1% | ❌ |
vxingest.grib2_to_cb | 13% | 1% | ❌ |
vxingest.netcdf_to_cb | 13% | 1% | ❌ |
vxingest.partial_sums_to_cb | 12% | 1% | ❌ |
vxingest.utilities | 30% | 32% | ❌ |
Summary | 16% (488 / 2989) | 6% (40 / 684) | ❌ |
I will add this to adb-cb1 cron after merge request is approved. Documentation in README.md