Make GitHub Versioner configurable. #236
Conversation
Integralist
requested review from
phamann,
a team,
triblondon and
cratelyn
and removed request for
a team
pkg/update/versioner.go
Outdated
| @@ -135,7 +139,7 @@ func (g GitHub) getReleaseID(ctx context.Context, version semver.Version) (id in | |||
| } | |||
|
|
|||
| func (g GitHub) getAssetID(assets []github.ReleaseAsset, version semver.Version) (id int64, err error) { | |||
| target := fmt.Sprintf("fastly_v%s_%s-%s.tar.gz", version, runtime.GOOS, runtime.GOARCH) | |||
| target := fmt.Sprintf("%s_v%s_%s-%s.tar.gz", g.org, version, runtime.GOOS, runtime.GOARCH) | |||
There was a problem hiding this comment.
fastly here is possibly not a github org
There was a problem hiding this comment.
Agreed, fastly in this context is the name of the binary program not the org. This is also applicable to many references above in the LatestVersion() method above, thus similar comments have been elided.
Furthermore, the filename pattern fastly_v0.26.2_linux-amd64.tar.gz is tightly coupled to the CLI release implementation and thus very opinionated and might not be the pattern that Viceroy or other future binaries use. Easy option is we ensure Viceroy uses this pattern or we will have to give some thought and refactoring as to how we generate the asset filename.
There was a problem hiding this comment.
@cratelyn 👋🏻 would you be able to offer any insights here ^^ re: filename pattern. Thanks.
There was a problem hiding this comment.
I think that this filename pattern sounds okay to me. Using semantic versioning would be fine, and putting a target/arch at the end is definitely worthwhile. No objections here!
There was a problem hiding this comment.
Per the inline discussion, I think we need to put more thought into how the filenames are constructed to be forwards compatible.
Personal preference/opinion, but I also found the renaming of a local variable s/versioner/cliVersioner/g unnecessary and made the review harder than it needed to be.
pkg/update/versioner.go
Outdated
| @@ -135,7 +139,7 @@ func (g GitHub) getReleaseID(ctx context.Context, version semver.Version) (id in | |||
| } | |||
|
|
|||
| func (g GitHub) getAssetID(assets []github.ReleaseAsset, version semver.Version) (id int64, err error) { | |||
| target := fmt.Sprintf("fastly_v%s_%s-%s.tar.gz", version, runtime.GOOS, runtime.GOARCH) | |||
| target := fmt.Sprintf("%s_v%s_%s-%s.tar.gz", g.org, version, runtime.GOOS, runtime.GOARCH) | |||
There was a problem hiding this comment.
Agreed, fastly in this context is the name of the binary program not the org. This is also applicable to many references above in the LatestVersion() method above, thus similar comments have been elided.
Furthermore, the filename pattern fastly_v0.26.2_linux-amd64.tar.gz is tightly coupled to the CLI release implementation and thus very opinionated and might not be the pattern that Viceroy or other future binaries use. Easy option is we ensure Viceroy uses this pattern or we will have to give some thought and refactoring as to how we generate the asset filename.
Integralist
force-pushed
the
integralist/20210325_configure_versioner
branch
from
f6bcc5a
to
50394dd
Compare
|
@phamann @triblondon looks like Viceroy team are happy with the file format used here and so are we unblocked on this PR now? |
Integralist
force-pushed
the
integralist/20210325_configure_versioner
branch
from
50394dd
to
d1013c7
Compare
Problem: We want to start integrating a new binary for local testing, which will be published as a public GitHub release, but the current Versioner type has hardcoded references to the CLI binary.
Notes: Because in future there will be two instances of a Versioner type in the code, I decided it best to rename all references to the variable
vorversionerto becliVersionerso it's clearer what versioner instance is being referred to.The primary change is in
pkg/update/versioner.go