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

Fetch redirected urls #900

Merged
merged 3 commits into from
Oct 18, 2018
Merged

Fetch redirected urls #900

merged 3 commits into from
Oct 18, 2018

Conversation

faustinoaq
Copy link
Contributor

Description of the Change

This PR adds the ability to fetch redirected urls like the used on recipes release page

Alternate Designs

Use redirect option on HTTP::Client (not supported yet, see: crystal-lang/crystal#2721)

Benefits

Fixes #899

Possible Drawbacks

No

Copy link
Contributor

@eliasjpr eliasjpr left a comment

Choose a reason for hiding this comment

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

:shipit:

@faustinoaq
Copy link
Contributor Author

faustinoaq commented Jul 6, 2018

This works but is pretty hacky, What would happens if the URL redirects 3 or more times?

I think we need something like HTTP::Client.get("<url>", redirects: true) @asterite @RX14 @straight-shoota @ysbaddaden, WDYT?

@asterite
Copy link

asterite commented Jul 6, 2018

There's an open issue for that in the stdlib. Someone just has to implement it (and it needs to be discussed how to implement it, which will probably result in it becoming a monster of complexity)

@drujensen
Copy link
Member

@faustinoaq should we merge this or should this wait?

@drujensen
Copy link
Member

Closing this. Recipes will no longer be hosted by Amber. We will work on allowing a recipe to be provided by a shard in a git repository.

@drujensen drujensen closed this Oct 14, 2018
@drujensen drujensen reopened this Oct 18, 2018
@drujensen drujensen merged commit 6400f7f into amberframework:master Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants