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

Enable https tests #379

Merged
merged 1 commit into from Jul 5, 2021
Merged

Enable https tests #379

merged 1 commit into from Jul 5, 2021

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Apr 30, 2021

closes #8677

@pulpbot
Copy link
Member

pulpbot commented Apr 30, 2021

Attached issue: https://pulp.plan.io/issues/8677

@@ -146,6 +146,13 @@ fi
ansible-playbook build_container.yaml
ansible-playbook start_container.yaml

# Hack: adding pulp CA to certifi.where()
sudo docker cp pulp:/etc/pulp/certs/ca.crt /usr/local/share/ca-certificates/pulp_ca.crt
CERT=$(python -c 'import certifi; print(certifi.where())')
Copy link
Member Author

Choose a reason for hiding this comment

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

This is Mozilla CA bundle, and I'm appending pulp CA to it

@@ -94,7 +94,7 @@ if [[ "$TEST" = 'bindings' || "$TEST" = 'publish' ]]; then
gem install --both ./{{ plugin_snake }}_client-0.gem
cd ..
{%- endif %}
ruby $REPO_ROOT/.ci/assets/bindings/test_bindings.rb
SSL_CERT_FILE=/usr/local/share/ca-certificates/pulp_ca.crt ruby $REPO_ROOT/.ci/assets/bindings/test_bindings.rb
Copy link
Member Author

Choose a reason for hiding this comment

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

Ruby doesn't use certifi lib, so I had to use an OpenSSL env var to point to the CA

@fao89 fao89 force-pushed the 8677 branch 6 times, most recently from 10e305b to 9a02955 Compare May 6, 2021 20:00
# Copy pulp CA
sudo docker cp pulp:/etc/pulp/certs/pulp_webserver.crt /usr/local/share/ca-certificates/pulp_webserver.crt
sudo docker cp pulp:/etc/pulp/certs/pulp_webserver.crt /etc/ssl/certs/pulp_webserver.crt
sudo docker cp pulp:/etc/pulp/certs/pulp_webserver.key /etc/ssl/private/pulp_webserver.key
Copy link
Member

Choose a reason for hiding this comment

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

I think, we don't need the key here. We should not expose it more than needed.

Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Quite a change... But it looks good to me!

Copy link
Contributor

@daviddavis daviddavis left a comment

Choose a reason for hiding this comment

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

This is awesome. Thank you!

@fao89 fao89 marked this pull request as ready for review May 13, 2021 17:02
@fao89
Copy link
Member Author

fao89 commented May 13, 2021

Since this is approved should we merge it?
About the single container part pulp/pulp-oci-images#73 I really don't know what to do, maybe we should decide on the next CI/CD meeting

@mdellweg
Copy link
Member

One last thing to consider: If we apply this template to old release branches, will they have a problem?
There is nothing that sticks out to me, however.

@daviddavis
Copy link
Contributor

We could open a pr and find out?

@fao89
Copy link
Member Author

fao89 commented May 14, 2021

One last thing to consider: If we apply this template to old release branches, will they have a problem?
There is nothing that sticks out to me, however.

since we are deciding to have HTTP and HTTPS tags, I can change this PR for https be optional

@daviddavis
Copy link
Contributor

I think that makes sense. Some plugins (like pulp_ansible) required updates to their tests since they do pulp-to-pulp syncs, etc. and these test changes might be hard to backport to older release branches.

I would recommend we default to https being true.

@fao89 fao89 force-pushed the 8677 branch 3 times, most recently from b0273fd to 3b003fb Compare May 20, 2021 13:40
@fao89 fao89 force-pushed the 8677 branch 2 times, most recently from bd009fb to 18718b8 Compare May 31, 2021 15:18
@fao89
Copy link
Member Author

fao89 commented Jun 3, 2021

Blocked by pulp/pulp-oci-images#73 or pulp/pulp-oci-images#84

@daviddavis
Copy link
Contributor

Hey, can you attach this to #403?

@mdellweg mdellweg linked an issue Jun 21, 2021 that may be closed by this pull request
@fao89 fao89 force-pushed the 8677 branch 2 times, most recently from 5c64463 to f6129b0 Compare July 2, 2021 12:20
@@ -33,6 +33,7 @@ DEFAULT_SETTINGS = {
'docker_fixtures': False,
'docs_test': True,
'issue_tracker': 'redmine',
'pulp_scheme': 'http',
Copy link
Member Author

Choose a reason for hiding this comment

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

made http default, for now, so I can merge it.
After merging pulp/pulp-oci-images#73 or pulp/pulp-oci-images#84
we can change it to https

@fao89 fao89 merged commit 717bc18 into pulp:master Jul 5, 2021
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.

Enable HTTPS on CI tests
4 participants