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

Update cookbook plugin #96

Merged
merged 11 commits into from
Feb 11, 2022
Merged

Update cookbook plugin #96

merged 11 commits into from
Feb 11, 2022

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Feb 7, 2022

No description provided.

@fao89 fao89 force-pushed the updcook branch 4 times, most recently from 9dd23f5 to 0e07c84 Compare February 7, 2022 18:46
@gmbnomis
Copy link
Collaborator

gmbnomis commented Feb 7, 2022

@fao89 This looks like you are trying to get this plugin running again, which would be great. Let me know if I can help.

@fao89 fao89 force-pushed the updcook branch 9 times, most recently from 4fae371 to cd99bb4 Compare February 8, 2022 23:19
@fao89 fao89 marked this pull request as ready for review February 8, 2022 23:32
@fao89 fao89 requested a review from gmbnomis February 8, 2022 23:33
@fao89
Copy link
Member Author

fao89 commented Feb 8, 2022

@fao89 This looks like you are trying to get this plugin running again, which would be great. Let me know if I can help.

could you please review?

Copy link
Collaborator

@gmbnomis gmbnomis left a comment

Choose a reason for hiding this comment

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

Nice work! However, I would like to understand the problems with test_contentview.py better.

set -euv


# This test is a mix between the functional tests and the unit tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure why you consider this test to be be a functional test as well. Looking at it, it does

from pulp_cookbook.tests.functional.constants import COOKBOOK_CONTENT_PATH

which it should not do. Are there other aspects that are causing problems in this test?

Copy link
Member Author

@fao89 fao89 Feb 10, 2022

Choose a reason for hiding this comment

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

it seems to do some API calls: https://github.com/pulp/pulp_cookbook/blob/main/pulp_cookbook/tests/unit/test_contentview.py#L67

We run unit tests inside the container and functional tests outside,
https://github.com/pulp/pulp_cookbook/runs/5116620108?check_suite_focus=true
I tried copying pulp-smash config to the container, but it broke:
https://github.com/pulp/pulp_cookbook/runs/5117392394?check_suite_focus=true
because pulp-smash assumes it is running outside of the container,
so I decided to skip this test until I figure out how to run it on CI.

Copy link
Collaborator

@gmbnomis gmbnomis Feb 10, 2022

Choose a reason for hiding this comment

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

it seems to do some API calls: https://github.com/pulp/pulp_cookbook/blob/main/pulp_cookbook/tests/unit/test_contentview.py#L67

It shouldn't do any real calls as this client is the APIClient from the REST framework, which acts as a "dummy" client simulating requests to views. In a pulplift environment this test works when Pulp is shut down.

We run unit tests inside the container and functional tests outside, https://github.com/pulp/pulp_cookbook/runs/5116620108?check_suite_focus=true I tried copying pulp-smash config to the container, but it broke: https://github.com/pulp/pulp_cookbook/runs/5117392394?check_suite_focus=true because pulp-smash assumes it is running outside of the container, so I decided to skip this test until I figure out how to run it on CI.

I hope that removing the functional test & pulp-smash dependency will suffice. As we only depend on COOKBOOK_CONTENT_PATH, we could set the constant here directly:

COOKBOOK_CONTENT_PATH = "/pulp/api/v3/content/cookbook/cookbooks/"

Worth a try IMHO. If that does not work, let's remove the test and deal with is separately as you propose. wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

it worked!
Thank you @gmbnomis

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.

2 participants