GeoNet / fits

Field Time Series
Apache License 2.0
4 stars 13 forks source link

feat: add github workflow #218

Closed BobyMCbobs closed 1 year ago

BobyMCbobs commented 1 year ago

to replace travis jobs

Proposed Changes

Resolves #.

Changes proposed in this pull request:

Production Changes

The following production changes are required to deploy these changes:

Review

Check the box that applies to this code review. If necessary please seek help with adding a checklist guide for the reviewer. When assigning the code review please consider the expertise needed to review the changes.

BobyMCbobs commented 1 year ago

Having a hard time getting this to build. Dapper should be separated out. Keeping on failing on go install ... https://github.com/GeoNet/fits/actions/runs/5686508902/job/15413410146?pr=218#step:18:208

Why is there's a separate dockerbuild.sh in ./dapper? https://github.com/GeoNet/fits/blob/main/dapper/dockerbuild.sh

it appears to not be consumed in the travis job

CallumNZ commented 1 year ago

@junghao I've always wondered why dapper sits in fits like this (if it fits it sits?). @BobyMCbobs are you suggesting it should sit inside cmd with fits-api ?

That dockerbuild.sh is vestigial, can be deleted.

junghao commented 1 year ago

@CallumNZ @BobyMCbobs https://github.com/GeoNet/fits/blob/main/build.sh#L27 The build script only use /cmd/fits-api, all the rest commands using /dapper/cmd/....

Why dapper under fits? Check the readme https://github.com/GeoNet/fits/tree/main/dapper. The purpose of dapper is to replace fits (see how long we've been hating fits). Unfortunately it never happened.

BTW, this is a public repo. So don't ask too much :D

BobyMCbobs commented 1 year ago

@junghao I've always wondered why dapper sits in fits like this (if it fits it sits?).

:laughing:

@BobyMCbobs are you suggesting it should sit inside cmd with fits-api ?

yeah, that'd be a good idea

That dockerbuild.sh is vestigial, can be deleted.

Sounds like a plan

BobyMCbobs commented 1 year ago

@CallumNZ @BobyMCbobs https://github.com/GeoNet/fits/blob/main/build.sh#L27 The build script only use /cmd/fits-api, all the rest commands using /dapper/cmd/....

:+1:

Why dapper under fits? Check the readme https://github.com/GeoNet/fits/tree/main/dapper. The purpose of dapper is to replace fits (see how long we've been hating fits). Unfortunately it never happened.

I might recommend a new repo for dapper, if possible.

BTW, this is a public repo. So don't ask too much :D

BobyMCbobs commented 1 year ago

I'm really unsure why one dapper build job passes while another fails.

BobyMCbobs commented 1 year ago

The structure including Dapper is too complicated for this flow to do a go build smoke test just for dapper and exclude dapper-send-test. I'm going to remove it for now. Dapper should be in a different repo in the future where there's an appropriate go.mod in the root of the repo

cc @junghao

BobyMCbobs commented 1 year ago

@junghao @sue-h-gns, thank you!