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

Adding nightly FIPS tests #345

Merged
merged 1 commit into from
Mar 2, 2021
Merged

Adding nightly FIPS tests #345

merged 1 commit into from
Mar 2, 2021

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Feb 24, 2021

@pulpbot
Copy link
Member

pulpbot commented Feb 24, 2021

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

@fao89
Copy link
Member Author

fao89 commented Feb 24, 2021

I'll be testing on pulp_npm:
pulp/pulp_npm#72

@fao89
Copy link
Member Author

fao89 commented Feb 25, 2021

@daviddavis
Copy link
Contributor

This work looks great. 💯

I see that the centos 7 test is failing against pulp_npm and pulp_ansible?

cd ../pulp_installer/
git submodule update --init
{%- if plugin_name in ['pulp_file', 'pulpcore'] %}
sed -i "/{{ plugin_name }}/d" .github/workflows/scripts/install.sh
Copy link
Member

Choose a reason for hiding this comment

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

This looks dangerous to me. Looking at these lines here, it's not even clear to me, what lines are supposed to be deleted. And if that file introduces a new important line containing "pulpcore" it will break in a way that is almost impossible to debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, it currently deletes git clone of pulpcore and pulp_file, I plan to change the script on the installer side, make it more customizable there

Copy link
Member

Choose a reason for hiding this comment

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

Being customizable in the script is great, because then you can see there that there might be an expected interface to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've started a PR: pulp/pulp_installer#539

@fao89
Copy link
Member Author

fao89 commented Mar 1, 2021

This work looks great.

I see that the centos 7 test is failing against pulp_npm and pulp_ansible?

yes, it failed all over the weekend, I've noticed GHA on my fork is behaving differently,
on my fork, it is already on ubuntu 20, so I pinned ubuntu 20 here expecting to get the same results, but it didn't work.
I suspect my fork is using a most updated runner

@fao89
Copy link
Member Author

fao89 commented Mar 1, 2021

Actually, they are using the same version

pulp:
image

fork:
image

I need to investigate more

@daviddavis
Copy link
Contributor

LGTM. @mdellweg did you want to review again before we merge?

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.

Rereview looks good.

plugin-template Outdated
@@ -48,6 +48,7 @@ DEFAULT_SETTINGS = {
'redmine_project': None,
'test_bindings': True,
'test_cli': False,
'fips_test': True,
Copy link
Member

Choose a reason for hiding this comment

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

Is True the proper default here? I know we want that on most plugins, but usually we add features disabled by default here.
(e.g. redmine_project, test_cli, test_performance, test_released_plugin_with_next_pulpcore_release, test_s3, ...)

edit: I see, that i 👍d the idea to add it True... So don't see this as a blocker.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested true by default but i think you’re right.

# runs at 4:00 UTC daily
- cron: '00 4 * * *'
{%- else -%}
workflow_dispatch:
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean, that you can still trigger the test by hand?
Can we rename the variable to test_fips_nightly to reflect that?

cd ../pulp_installer/
git submodule update --init
{%- if plugin_name in ['pulp_file', 'pulpcore'] %}
sed -i "/{{ plugin_name }}/d" .github/workflows/scripts/install.sh
Copy link
Member

Choose a reason for hiding this comment

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

Being customizable in the script is great, because then you can see there that there might be an expected interface to it.

run: |
cd ../pulp_installer
{% if plugin_name not in ['pulp_file', 'pulpcore'] -%}
sed -e "s/pulp-file/{{ plugin_dash }}/g" -e "s/pulp_file/{{ plugin_name }}/g" -e "/pulp_source_dir/a pulp_webserver_disable_https: true" example.dev-config.yml > local.dev-config.yml
Copy link
Member

Choose a reason for hiding this comment

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

👍
Maybe in a next iteration, we should just provide the proper dev-config in the assets directory. It can use the additional_plugins variable at templating to determine what to install.

@fao89 fao89 merged commit 6a0ba09 into pulp:master Mar 2, 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.

None yet

4 participants