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

add download option to jenkins module #1047

Closed
wants to merge 9 commits into from

Conversation

CyberLine
Copy link
Contributor

While on Debian10 plugin installation very often fails with:

Error: /Stage[main]/Jenkins::Plugins/Jenkins::Plugin[conditional-buildstep]/Archive[conditional-buildstep.hpi]/ensure: change from 'absent' to 'present' failed: Execution of '/usr/bin/curl https://updates.jenkins.io/latest/conditional-buildstep.hpi -o /tmp/conditional-buildstep.hpi_20220728-1598563-1n3z4vb -fsSLg --max-redirs 5' returned 16: curl: (16) Error in the HTTP2 framing layer

i added http1.1 hard as download option to prevent those errors.

@@ -56,6 +59,7 @@
Boolean $enabled = true,
String $digest_type = 'sha1',
Boolean $pin = false,
Array $download_options = ['--http1.1'],
Copy link
Member

Choose a reason for hiding this comment

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

Should this really be the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the error is depending on which server behind update.jenins.io is serving the request. I think there is no downside for now to always use http1.1 instead of 2.0?

Copy link
Member

Choose a reason for hiding this comment

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

HTTP 2.0 can be more efficient. Have you at least filed an issue with Jenkins to track it? IMHO such a workaround always needs an issue and link it in a code comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there are a few servers behind and one of them is randomly having issues. i still haven't found out which server.
For me it will also be fine to remove the default but make it injectable via parameter so i can override it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that certainly helps in giving me assurances that this is the right thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well so there is the question if i make this exposing to for example jenkins::params::plugin_download_options ?

And why the heck are the tests failing since ages?

Copy link
Member

Choose a reason for hiding this comment

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

And why the heck are the tests failing since ages?

It has been very hard to keep them green, mostly because we don't have proper dependency resolution. Ideally we'd have something that utilizes https://github.com/jenkinsci/plugin-installation-manager-tool/ so you don't need to do that yourself.

Copy link
Contributor Author

@CyberLine CyberLine Jul 28, 2022

Choose a reason for hiding this comment

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

As i can see in the documentation, the tool also won't resolve dependencies for you :(
For me my current plugin setup looks like this for latest debian version: (snippet)
image
It's a mess ..

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.

It took me a while, but I've finally found time to fix the tests. Please rebase.

manifests/job.pp Outdated
@@ -13,7 +13,7 @@
define jenkins::job (
String $config,
Optional[String] $source = undef,
Optional[Stdlib::Absolutepath] $template = undef,
Optional[String] $template = undef,
Copy link
Member

Choose a reason for hiding this comment

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

Could you submit this as a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in 6971930

@ekohl
Copy link
Member

ekohl commented Apr 19, 2023

I'm sorry I missed the rebase, and now there's another conflict. I'm also running into this, so I opened #1073 as a clean rebase with a better commit message.

@evgeni
Copy link
Member

evgeni commented Apr 24, 2024

#1073 was merged

@evgeni evgeni closed this Apr 24, 2024
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.

4 participants