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.

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

Allow local binary to be renamed. #240

Merged
merged 1 commit into from Apr 7, 2021

Conversation

Integralist
Copy link
Collaborator

Problem: as we integrate a new binary for C@E local testing we don't want to confuse users by downloading the binary using its internal name.
Solution: support renaming the binary once it has been extracted from the release. Something like...

org := "fastly"
repo := "viceroy"
binary := "viceroy"

viceroyVersioner = update.NewGitHub(context.Background(), org, repo, binary)
viceroyVersioner.RenameLocalBinary("fastly-localtesting")

@Integralist Integralist added the enhancement New feature or request label Mar 29, 2021
@Integralist Integralist requested review from phamann, a team, kailan and cratelyn and removed request for a team March 29, 2021 17:04
@kailan
Copy link
Member

kailan commented Mar 30, 2021

My thoughts on this as discussed with @Integralist:

  • If Viceroy is being pulled from GitHub releases, as this PR suggests, then it seems like the binary being named "viceroy" would not be an issue. A quick search for "fastly viceroy" will answer the question of what it is.
  • If Viceroy is staying as a private repo, I would suggest renaming the binary on the update server rather than downloading it and then renaming locally.

Out of interest, where will viceroy be downloaded to? I assumed the config dir (~/Library/Application Support/fastly on macOS), in which case I would not worry about file naming. If they are confident enough to poke around in system dirs and delete a random binary, it should be obvious what has happened when fastly compute serve stops working.

Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

This looks good to me! While I think I do agree with @kailan's point above, the ability to rename a local binary ourselves seems like a worthwhile addition.

@Integralist Integralist force-pushed the integralist/20210329_viceroy_binary_name branch from b7193f9 to 3713057 Compare April 1, 2021 15:11
@Integralist
Copy link
Collaborator Author

Thanks @cratelyn

@kailan I think under the circumstances I'm OK to merge this PR then. Just wanted to give you one last opportunity to holla if you were strongly opposed.

@Integralist Integralist force-pushed the integralist/20210329_viceroy_binary_name branch from 3713057 to 9d982bb Compare April 7, 2021 08:21
@Integralist Integralist force-pushed the integralist/20210329_viceroy_binary_name branch from 9d982bb to c142483 Compare April 7, 2021 08:43
@Integralist Integralist merged commit aec32cf into master Apr 7, 2021
@Integralist Integralist deleted the integralist/20210329_viceroy_binary_name branch April 7, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants