Add `http2` directive instead of `listen` option #1579

C24-AK · 2023-10-17T13:27:47Z

Pull Request (PR) description

This PR is meant to create the http2 directive: http://nginx.org/en/docs/http/ngx_http_v2_module.html#http2

This Pull Request (PR) fixes the following issues

Fixes following warning in nginx:
nginx: [warn] the "listen ... http2" directive is deprecated, use the "http2" directive instead.

kenyon · 2023-10-17T23:45:39Z

The tests need to be updated to match this change.

C24-AK · 2023-10-18T12:38:54Z

I can't test this with these containers, since nginx is getting pulled from nginx repo which does not have the latest nginx package. Locally we are testing with latest version, since it is required for this PR.
@kenyon how do I proceed from here?

kenyon · 2023-10-19T20:19:42Z

You can specify an nginx version in an acceptance test's puppet code, just like in regular puppet code. Since the http2 directive doesn't exist in older versions, I think you have to have some logic that only uses it if the nginx version is 1.25.1 or newer.

C24-AK · 2023-10-24T12:51:46Z

@kenyon Hey, can you tell me why archlinux is the only test failing? It seems like other Pull Requests have the same problem.

kenyon · 2023-10-24T21:14:06Z

@C24-AK Archlinux test failure is expected for now.

C24-AK · 2023-11-01T10:42:01Z

@kenyon Hey, what to do on following error?
This happens outside of examples:

JSON::ParserError:
unexpected token at ''
./vendor/bundle/ruby/2.7.0/gems/json-2.6.3/lib/json/common.rb:216:in parse' ./vendor/bundle/ruby/2.7.0/gems/json-2.6.3/lib/json/common.rb:216:in parse'
./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:99:in load_filters' ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:80:in block in merge_filters'
./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:79:in each' ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:79:in merge_filters'
./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:201:in run_report' ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:186:in report!'
./vendor/bundle/ruby/2.7.0/gems/voxpupuli-test-7.0.2/lib/voxpupuli/test/spec_helper.rb:21:in `block (2 levels) in <top (required)>'

kenyon · 2023-11-01T20:41:30Z

Yeah I don't know what is causing that. Some debugging required.

C24-AK · 2023-11-02T08:01:14Z

Is this something I need to fix for this PR to be merged?

templates/server/server_ssl_header.erb

smortex · 2023-11-03T17:12:01Z

That looks better, thanks! (Edit: The archlinux failure seems unrelated)

Can you please combine your 40+ commits in a single one?

From your working directory:

git rebase -i origin/main # Interactively rewrite history

This will bring your editor listing all commits in your branch. Keep the first line unchanged, and replace pick with squash for all subsequent lines. Save and quit, your editor will pop again and allow you to combine all the commit messages in a single one.

Then push your updated branch:

git push -f            # Send the changes (-f is required because we re-wrote history)

C24-AK · 2023-11-06T07:55:00Z

Hey @smortex,

I did what you explained and it feels like all my changes are gone?
All tests are failing

Edit: I reverted it, should be fixed.

adjusted http2 checks adding nginx version to acceptance tests revert changes set default version to 1.25.2 adjusted docs revert changes Test with repository which contains the latest version Test with default repo source and manage repo true Test without setting version Test with 1.25.1 Debugging facts Debugging facts Debugging facts add version to mail fix a test for 1.14 testing testing Debug and tracing set package_ensure disable rubocop disable rubocop set package to altest set package to latest mailspec remove tracing set http2 set http2 directive mailhost mailhost tracing revert revert revert http2 tests http2 tests http2 tests testing testing testing http2 test http2 test http2 test http2 test Fix http2 for old versions

C24-AK · 2023-11-09T08:39:36Z

@smortex anything else to do or am I finished here?

spec/defines/resource_server_spec.rb

smortex · 2023-11-09T08:46:10Z

@smortex anything else to do or am I finished here?

That look good, the failures seems unrelated, I tried to run the test again, maybe it will pass and if not maybe this will ring a bell to some other contributor?

I added an in-line note about old test removal, and I wonder if we can keep them for now?

C24-AK · 2023-11-09T12:08:28Z

@smortex I resolved your comment and added the tests

kenyon · 2023-11-10T01:09:43Z

I just reran the failing Puppet 7 job to see if it's consistent.

kenyon · 2023-11-10T01:43:25Z

I'm not sure how this change would be causing this error, but it seems like it is.

From https://github.com/voxpupuli/puppet-nginx/actions/runs/6809552993/job/18546775611?pr=1579#step:5:299

An error occurred in an `after(:suite)` hook.
Failure/Error: Parser.new(source, **(opts||{})).parse

JSON::ParserError:
  unexpected token at ''
# ./vendor/bundle/ruby/2.7.0/gems/json-2.6.3/lib/json/common.rb:216:in `parse'
# ./vendor/bundle/ruby/2.7.0/gems/json-2.6.3/lib/json/common.rb:216:in `parse'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:99:in `load_filters'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:80:in `block in merge_filters'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:79:in `each'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:79:in `merge_filters'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:201:in `run_report'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:186:in `report!'
# ./vendor/bundle/ruby/2.7.0/gems/voxpupuli-test-7.0.2/lib/voxpupuli/test/spec_helper.rb:21:in `block (2 levels) in <top (required)>'

C24-AK · 2023-11-10T08:32:18Z

@kenyon I am not very familiar with this. How would I fix this issue? I appreciate any help

C24-AK · 2023-11-13T08:12:49Z

Is this rdy to merge? @kenyon

smortex reviewed Nov 2, 2023

View reviewed changes

templates/server/server_ssl_header.erb Outdated Show resolved Hide resolved

C24-AK force-pushed the feature/http2-directive branch from 53d0a54 to 62e870e Compare November 6, 2023 07:06

C24-AK force-pushed the feature/http2-directive branch from 62e870e to a0adee7 Compare November 6, 2023 08:00

smortex reviewed Nov 9, 2023

View reviewed changes

spec/defines/resource_server_spec.rb Outdated Show resolved Hide resolved

add http2 tests

b2c7a63

kenyon mentioned this pull request Nov 10, 2023

Add support for multiple limit_req statements in location directives #1570

Merged

kenyon added the enhancement New feature or request label Nov 13, 2023

kenyon changed the title ~~added http2 directive instead of listen option~~ Add http2 directive instead of listen option Nov 13, 2023

kenyon merged commit c6ab9de into voxpupuli:master Nov 13, 2023
25 of 27 checks passed

kenyon mentioned this pull request May 26, 2024

[warn] the "listen ... http2" directive is deprecated, use the "http2" directive instead in [from nginx 1.25.1 onwards] #1593

Closed

TheMeier added this to the 6.0.0 milestone Jun 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `http2` directive instead of `listen` option #1579

Add `http2` directive instead of `listen` option #1579

C24-AK commented Oct 17, 2023

kenyon commented Oct 17, 2023

C24-AK commented Oct 18, 2023

kenyon commented Oct 19, 2023

C24-AK commented Oct 24, 2023

kenyon commented Oct 24, 2023

C24-AK commented Nov 1, 2023

kenyon commented Nov 1, 2023

C24-AK commented Nov 2, 2023

smortex commented Nov 3, 2023 •

edited

Loading

C24-AK commented Nov 6, 2023 •

edited

Loading

C24-AK commented Nov 9, 2023

smortex commented Nov 9, 2023

C24-AK commented Nov 9, 2023

kenyon commented Nov 10, 2023

kenyon commented Nov 10, 2023

C24-AK commented Nov 10, 2023

C24-AK commented Nov 13, 2023

Add http2 directive instead of listen option #1579

Add http2 directive instead of listen option #1579

Conversation

C24-AK commented Oct 17, 2023

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

kenyon commented Oct 17, 2023

C24-AK commented Oct 18, 2023

kenyon commented Oct 19, 2023

C24-AK commented Oct 24, 2023

kenyon commented Oct 24, 2023

C24-AK commented Nov 1, 2023

kenyon commented Nov 1, 2023

C24-AK commented Nov 2, 2023

smortex commented Nov 3, 2023 • edited Loading

C24-AK commented Nov 6, 2023 • edited Loading

C24-AK commented Nov 9, 2023

smortex commented Nov 9, 2023

C24-AK commented Nov 9, 2023

kenyon commented Nov 10, 2023

kenyon commented Nov 10, 2023

C24-AK commented Nov 10, 2023

C24-AK commented Nov 13, 2023

Add `http2` directive instead of `listen` option #1579

Add `http2` directive instead of `listen` option #1579

smortex commented Nov 3, 2023 •

edited

Loading

C24-AK commented Nov 6, 2023 •

edited

Loading