exercism / v2-configlet

Tool to assist in managing Exercism language tracks.
MIT License
16 stars 23 forks source link

Add upgrade command #112

Closed kytrinyx closed 6 years ago

kytrinyx commented 6 years ago

We have an upgrade command in the Exercism CLI, which would be handy to have in Configlet.

I kind of don't want to extract that logic into a separate package and create a whole new dependency that we need to track, so I'm tempted to suggest that we copy it in.

On the other hand, copy/paste might be a terrible idea.

What do you all think?

tleen commented 6 years ago

Gut feeling is copy/paste is a terrible idea. But my general rule has been (perhaps) copy paste once, but never twice. Do you think any other aspect of the tooling may need an auto-upgrade?

nywilken commented 6 years ago

I would like to look into this further. I peeked at the cli pkg within nextercism branch which is responsible for upgrading nextercism. It is written in a way where we could import that pkg by itself and write a slightly modified upgrade command. Does anyone mind if I take a stab at this?

kytrinyx commented 6 years ago

Does anyone mind if I take a stab at this?

That would be lovely!

nywilken commented 6 years ago

@kytrinyx thanks, I have a WIP at https://github.com/nywilken/configlet/pull/2. I'm running into an issue when trying to extract the latest release -- see below

> configlet version
configlet version 3.6.0
-> There is a newer version of Configlet available (3.6.1)

vinewood [0] go-1.9.2 in ~/Development/golang/src/github.com/exercism/configlet/ on nywilken/upgrade-command (ahead 5)
> configlet upgrade -v 
Downloading configlet-linux-64bit.tgz
Error: gzip: invalid header

Looking at the response I get back from GitHub's release API I see that the Linux assets have a content-type of "application/x-compressed-tar", where as on the CLI repo they have "application/gzip". Have you come across this issue before?

kytrinyx commented 6 years ago

Have you come across this issue before?

I have not! That's very odd. They're built in the same way, so I have no idea why they'd get a different header.

Would you happen to be able to do a curl -v request to reproduce this? If so I can pass this on at work (or debug it when I get back).

nywilken commented 6 years ago

@kytrinyx so I dug in a little further and found the issue. It turns out that there is a hard coded URL in the download method for cli.Asset, which responds with a 404 when executed via Configlet's upgrade command because it points to a different repo.

Downloading configlet-linux-64bit.tgz
application/x-compressed-tar // We set Accept:  
{"message":"Not Found","documentation_url":"https://developer.github.com/v3/repos/releases/#get-a-single-release-asset"}

I changed the download method for Asset to support configlet, as a test, and things work as expected. I will continue with things and will most likely open up a PR to the CLI project to support different Asset Release URLs. Thanks for the help!

kytrinyx commented 6 years ago

Nice find! Thanks for digging into that.