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

I removed wget in favor of curl. #343

Closed
wants to merge 8 commits into from
Closed

Conversation

tdensmore
Copy link
Contributor

Often people don't have wget. wget is easy enough to install, but curl is more widely available by default.

This commit will need some Windows testing love, since I don't have Windows to test with.

@AmberAlston
Copy link
Member

@tdensmore @dexhorthy Are you still planning to push forward on this requested content change?

@tdensmore
Copy link
Contributor Author

tdensmore commented Jan 8, 2021

@AmberAlston I would love to get this released. What do I need to do?

@AmberAlston
Copy link
Member

@tdensmore Since you requested @dexhorthy as a reviewer, push on him to help validate your suggested changes are useful improvements, and mark PR as reviewed. In the meantime you've got lots of quirky formatting (missing space between paragraphs, extra space between words and punctuation). Give it a once over on https://deploy-preview-343--kotsio.netlify.app/reference/v1beta1/application/ and fix the formatting issues. The current in-page headers in this pr are a big change in styling. If you intend to suggest that, please add @jgruica or @GraysonNull as a final reviewer post Dex review and your formatting fixes.

@tdensmore
Copy link
Contributor Author

I have updated the formatting @AmberAlston . @dexhorthy can you look at this and approve if you like it?

@tdensmore
Copy link
Contributor Author

Actually would either @cremerfc or @aj-jester be able to review these changes? They are minor: one functionality change (using curl instead of wget) and documentation changes to the kots.io website

@AmberAlston
Copy link
Member

Bump on request for review @cremerfc @aj-jester @dexhorthy

@AmberAlston
Copy link
Member

Kicking this PR again in case you want to move forward with it. If not, we're going to close it as stale in the near future.

@tdensmore
Copy link
Contributor Author

tdensmore commented Jun 1, 2021

Hey gang - I wanted to get some eyeballs on this before @AmberAlston marks it as stale. It is a minor change to remove wget in favor of curl ... @soriaj @jdewinne @cremerfc

Makefile Outdated
unzip hugo.zip -d deps; \
else \
wget -O hugo.tar.gz $(url).tar.gz; \
echo $(url).tar.gz; \
Copy link
Member

Choose a reason for hiding this comment

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

Why is the echo in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. it was probably for debugging. I will remove it.

@jdewinne
Copy link
Member

jdewinne commented Jun 1, 2021

I would split the wget to curl and the documentation changes into 2 separate PR's. Or are they related?

@tdensmore
Copy link
Contributor Author

I would split the wget to curl and the documentation changes into 2 separate PR's. Or are they related?

I will split these out... it has been so long that I forgot they were in there.

@tdensmore
Copy link
Contributor Author

I will close this in favor of the two new PRs I created. One for curl, the other for updated documentation.

@AmberAlston AmberAlston closed this Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants