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

Fixes #36138 - Invalid syntax for curl --time-cond #856

Merged
merged 1 commit into from Mar 9, 2023

Conversation

stejskalleos
Copy link
Contributor

@stejskalleos stejskalleos commented Feb 23, 2023

Fixes: 3d87c6f
According to Curl man page, syntax for --time-cond attribute should be --time-cond file <path>, not --time-cond <path>.

This can cause issues with outdated vmlinuz and initrd.img files,
once the files are downloaded, they are never replaced with never versions.

How to test

  • Provision host (baremetal, networking)
  • touch -a -m -t 201512180130.09 initrd.img & vmlinuz in /var/lib/tftpboot/boot
  • Provision another host with same OS
  • Check that files have been updated

[0] https://curl.se/docs/manpage.html

@stejskalleos
Copy link
Contributor Author

And I guess we should CP it to 3.5 stable too.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks for providing so much context. Made it really easy to review.

And I guess we should CP it to 3.5 stable too.

I always find out in which commit it was introduced. git blame -L 29,29 lib/proxy/http_download.rb quickly tells me it was last modified in 3d87c6f. Always verify it wasn't some patch that just changed formatting, but in this case it's obvious.

Then I add Fixes: $COMMIT to the commit message, which I saw in the Linux kernel. While looking for a link to their description I see they also include a description. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes & https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes have more.

So in this case it's 3d87c6f which introduced the bug, so then I check where it's included:

$ git tag --contains 3d87c6feaa8caaf9b8c040f96061c3b8c33a39ef
3.1.0
3.1.0-rc1
3.1.0-rc2
3.1.1
3.1.1.1
3.1.1.2
3.1.1.3
3.1.2
3.1.3
3.2.0
3.2.0-rc1
3.2.0-rc2
3.2.1
3.3.0
3.3.0-rc1
3.3.0-rc2
3.3.1
3.4.0
3.4.0-rc1
3.4.0-rc2
3.4.1
3.5.0
3.5.0-rc1
3.5.0-rc2
3.5.1

So this certainly needs to be picked to 3.5-stable, but also 3.4-stable and since we branched 3.6-stable yesterday that too.

lib/proxy/http_download.rb Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks! Small nit: it's Fixes: COMMIT instead of Fixes COMMIT. https://git-scm.com/docs/git-interpret-trailers isn't very clear, but the core is that they're supposed be parseable. A more concrete example is 22282fc, which you can then parse:

$ git show 22282fc7483f7a2439cc69c57d361860ea221fc9 --format="%(trailers)" --quiet
Fixes: aa25357d4404908d5e5707e767ecd798b754d7d1

We don't have it today, but this could then be used in automation to determine cherry picks. That's still on my wish list.

I believe it is also needed to put it at the end, separated by a newline from the main body.

According to Curl man page, syntax for --time-cond attribute
should be "--time-cond file <path>", not "--time-cond <path>".

This can cause issues with outdated vmlinuz and initrd.im files,
once the files are downloaded, they are never replaced with never versions.

[0] https://curl.se/docs/manpage.html

Fixes: 3d87c6f
@Ron-Lavi
Copy link
Member

Ron-Lavi commented Mar 9, 2023

@ekohl can we merge this?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks!

@ekohl ekohl merged commit fba49ab into theforeman:develop Mar 9, 2023
stejskalleos added a commit to stejskalleos/smart-proxy that referenced this pull request Mar 22, 2023
PR theforeman#856 was fixing invalid curl --time-cont condition,
but it actually break it and now files are not downloaded at all.

This reverts commit fba49ab.
ekohl pushed a commit that referenced this pull request Mar 23, 2023
PR #856 was fixing invalid curl --time-cont condition,
but it actually break it and now files are not downloaded at all.

This reverts commit fba49ab.
ekohl pushed a commit to ekohl/smart-proxy that referenced this pull request Mar 23, 2023
PR theforeman#856 was fixing invalid curl --time-cont condition,
but it actually break it and now files are not downloaded at all.

This reverts commit fba49ab.

(cherry picked from commit 90d7cc8)
ekohl pushed a commit to ekohl/smart-proxy that referenced this pull request Mar 23, 2023
PR theforeman#856 was fixing invalid curl --time-cont condition,
but it actually break it and now files are not downloaded at all.

This reverts commit fba49ab.

(cherry picked from commit 90d7cc8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants