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

Drop unused ports and switch run_init to false #113

Merged
merged 2 commits into from
Nov 21, 2018

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Oct 12, 2018

From my testing and talking to Candlepin team:

  • Calling init is really a development task and not needed for production
  • AJP connector seems to be only useful if using Apache and even then not if you are using mod_proxy
  • The init can be called via HTTPS so 8080 is not needed at all

This reduces Candlepin setup time, and reduces the number of ports that Candlepin takes up.

@ehelms
Copy link
Member Author

ehelms commented Oct 12, 2018

@kahowell Would you mind verifying my assumptions here are correct?

I tested all these changes on Katello nightly and saw no degradation of testing.

@ehelms
Copy link
Member Author

ehelms commented Oct 15, 2018

I don't know that I understand these tests well enough to know why this fails to detect 8443 as the available port.

@ekohl
Copy link
Member

ekohl commented Oct 15, 2018

We're using a testing pattern that's not really the standard puppet way. Since we decided that this isn't for 1.20 I'll revisit this when RC1 is out.

Copy link
Contributor

@kahowell kahowell left a comment

Choose a reason for hiding this comment

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

From my point of view, this PR is 👍

@ekohl
Copy link
Member

ekohl commented Oct 26, 2018

You'll want to rebase on #116. If you could also hide this behind if $candlepin::run_init that'd make the default installation slightly smaller:

ensure_packages(['wget'], { ensure => $candlepin::wget_version, })

@ehelms ehelms force-pushed the drop-http branch 6 times, most recently from d40dfa8 to 3dff17d Compare October 30, 2018 11:56
@ehelms
Copy link
Member Author

ehelms commented Oct 30, 2018

Still a bit stuck after rebase on how to get this updated test to pass

@ekohl
Copy link
Member

ekohl commented Nov 7, 2018

From the logs:

Nov 07 17:31:03 centos7-64.example.com server[1730]: SEVERE: Failed to load keystore type PKCS12 with path conf/keystore due to /usr/share/tomcat/conf/keystore (No such file or directory)
Nov 07 17:31:03 centos7-64.example.com server[1730]: java.io.FileNotFoundException: /usr/share/tomcat/conf/keystore (No such file or directory)

I suspect the issue is that we never set up a keystore with a certificate in our tests so we can't listen on 8443. Previously we listened on 8080 and it was no problem.

@ekohl
Copy link
Member

ekohl commented Nov 12, 2018

@ehelms I've updated the example to generate a keystore as well

@ekohl
Copy link
Member

ekohl commented Nov 12, 2018

I've opened #119 to remove the lint check

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.

Should be good to go when the tests are green.

@ehelms
Copy link
Member Author

ehelms commented Nov 12, 2018

Thanks for all the help @ekohl ! I am thinking I either hold on merging this until Katello 3.10 is branched, or merge and don't update this module in 3.10

@ekohl
Copy link
Member

ekohl commented Nov 12, 2018

I'm fine with holding off on merging. Otherwise 3.10 will be a downgrade compared to nightly and I think that's uglier than the alternatives.

@ekohl
Copy link
Member

ekohl commented Nov 20, 2018

Before I forget: this will need an installer migration for upgrading users.

@ekohl
Copy link
Member

ekohl commented Nov 20, 2018

Before I forget: this will need an installer migration for upgrading users.

On second thought, it would if it was a top level module but it isn't.

@ehelms ehelms merged commit 96efae4 into theforeman:master Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants