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.

Sign up for GitHub

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

Jump to bottom

Fix broken curl in multiarch build + fix retry logic break #44

Merged
merged 4 commits into from Jul 7, 2020
Merged

Fix broken curl in multiarch build + fix retry logic break #44

merged 4 commits into from Jul 7, 2020

Conversation

migueleliasweb
Copy link
Contributor

@migueleliasweb migueleliasweb commented Jun 23, 2020 •
edited

In this PR:

  • Fix multi arch build (curl problem)
  • Fix retry logic that would break further reloads
  • Expose metric for exhausted retries

After searching a bit regarding this error, apparently apk does not bring the newest versions of some libs; you need to forcibly upgrade them. That fixes the curl problem during the multi arch build.

Before:

$ docker run -it --rm --entrypoint sh docker:19.03.1-git
/ # apk add -U make bash curl
fetch http://dl-cdn.alpinelinux.org/alpine/v3.10/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.10/community/x86_64/APKINDEX.tar.gz
(1/4) Installing readline (8.0.0-r0)
(2/4) Installing bash (5.0.0-r0)
Executing bash-5.0.0-r0.post-install
(3/4) Installing curl (7.66.0-r0)
(4/4) Installing make (4.2.1-r2)
Executing busybox-1.30.1-r2.trigger
OK: 33 MiB in 30 packages
/ # curl https://google.com
Error relocating /usr/bin/curl: curl_multi_poll: symbol not found

After:

$ docker run -it --rm --entrypoint sh docker:19.03.1-git
/ # apk add -U make bash curl
fetch http://dl-cdn.alpinelinux.org/alpine/v3.10/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.10/community/x86_64/APKINDEX.tar.gz
(1/4) Installing readline (8.0.0-r0)
(2/4) Installing bash (5.0.0-r0)
Executing bash-5.0.0-r0.post-install
(3/4) Installing curl (7.66.0-r0)
(4/4) Installing make (4.2.1-r2)
Executing busybox-1.30.1-r2.trigger
OK: 33 MiB in 30 packages
/ # apk upgrade # <--------------------------- this does the fix to the curl command
(1/15) Upgrading busybox (1.30.1-r2 -> 1.30.1-r4)
Executing busybox-1.30.1-r4.post-upgrade
(2/15) Upgrading libcrypto1.1 (1.1.1c-r0 -> 1.1.1g-r0)
(3/15) Upgrading libssl1.1 (1.1.1c-r0 -> 1.1.1g-r0)
(4/15) Upgrading ca-certificates-cacert (20190108-r0 -> 20191127-r2)
(5/15) Upgrading ssl_client (1.30.1-r2 -> 1.30.1-r4)
(6/15) Upgrading ncurses-terminfo-base (6.1_p20190518-r0 -> 6.1_p20190518-r2)
(7/15) Upgrading ncurses-libs (6.1_p20190518-r0 -> 6.1_p20190518-r2)
(8/15) Purging ncurses-terminfo (6.1_p20190518-r0)
(9/15) Upgrading ca-certificates (20190108-r0 -> 20191127-r2)
(10/15) Upgrading nghttp2-libs (1.38.0-r0 -> 1.39.2-r1)
(11/15) Upgrading libcurl (7.65.1-r0 -> 7.66.0-r0)
(12/15) Upgrading expat (2.2.7-r0 -> 2.2.8-r0)
(13/15) Upgrading git (2.22.0-r0 -> 2.22.4-r0)
(14/15) Upgrading openssh-keygen (8.0_p1-r0 -> 8.1_p1-r0)
(15/15) Upgrading openssh-client (8.0_p1-r0 -> 8.1_p1-r0)
Executing busybox-1.30.1-r4.trigger
Executing ca-certificates-20191127-r2.trigger
OK: 27 MiB in 29 packages
/ # curl https://google.com
<HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">
<TITLE>301 Moved</TITLE></HEAD><BODY>
<H1>301 Moved</H1>
The document has moved
<A HREF="https://www.google.com/">here</A>.
</BODY></HTML>

@migueleliasweb
Copy link
Contributor Author

Hey @jimmidyson , do you think if we merge this one we can get a new release? Maybe 0.4.0?

@migueleliasweb
Copy link
Contributor Author

Ps: In the README the link for Tidelift seems broken, are you still using it? If not, should it be removed?

@migueleliasweb migueleliasweb changed the title Fix broken curl in multiarch build Fix broken curl in multiarch build + fsnotify version Jun 24, 2020
@migueleliasweb migueleliasweb changed the title Fix broken curl in multiarch build + fsnotify version Fix broken curl in multiarch build + downgrade fsnotify version Jun 24, 2020
@migueleliasweb migueleliasweb changed the title Fix broken curl in multiarch build + downgrade fsnotify version + downgrade fsnotify version Jun 24, 2020
@migueleliasweb migueleliasweb changed the title + downgrade fsnotify version Fix broken curl in multiarch build + fix retry logic break Jun 24, 2020
@migueleliasweb
Copy link
Contributor Author

migueleliasweb commented Jun 24, 2020 •
edited

By using return, I broke the logic onde the first webhook succeeds. It was making the goroutine return and there was nothing to bring it back up. Using break instead fixes it. Sorry for that! 🙈

@migueleliasweb
Copy link
Contributor Author

Ping @jimmidyson

@migueleliasweb
Copy link
Contributor Author

Please 🙏 @jimmidyson

@migueleliasweb
Copy link
Contributor Author

Sorry to bother again @jimmidyson but could you have a look at this PR?

@jimmidyson
Copy link
Owner

Sorry for slow response! Thanks for fixing this.

@jimmidyson jimmidyson merged commit 73884be into jimmidyson:master Jul 7, 2020
@migueleliasweb
Copy link
Contributor Author

It's all good @jimmidyson , I was just cleaning up my mess! 😂

@migueleliasweb
Copy link
Contributor Author

@jimmidyson Do you think we can cut a new release for this merge? Maybe v.0.4.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmidyson jimmidyson jimmidyson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@migueleliasweb @jimmidyson