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.

Sign up for GitHub

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

Jump to bottom

Add Domain support. #3106

Merged
merged 1 commit into from
May 22, 2023
Merged

Add Domain support. #3106

merged 1 commit into from
May 22, 2023

Conversation

ggainey
Copy link
Contributor

@ggainey ggainey commented Apr 5, 2023

closes #3008

(cherry picked from commit 67067318fecf83e0bbe471739b052a1668c893df)
Co-authored by: Grant Gainey ggainey@redhat.com

@ggainey ggainey marked this pull request as draft April 5, 2023 18:02
@ggainey
Copy link
Contributor Author

ggainey commented Apr 5, 2023

This is very draft - pushing it up to make it possible to have eyes on it when/if/as I run into problems.

@ggainey ggainey force-pushed the 2920_domain_work branch 2 times, most recently from cd9991a to 690ece2 Compare April 5, 2023 18:29
@ggainey ggainey mentioned this pull request Apr 5, 2023
Closed
gerrod3
gerrod3 reviewed Apr 5, 2023
View reviewed changes
Copy link
Contributor

@gerrod3 gerrod3 left a comment

Choose a reason for hiding this comment

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

Not sure if you need any changes in the task code as well. If they use queries then they probably need a filter by task domain.

pulp_rpm/app/models/modulemd.py Outdated Show resolved Hide resolved
pulp_rpm/app/viewsets/acs.py Outdated Show resolved Hide resolved
@ggainey
Copy link
Contributor Author

ggainey commented Apr 5, 2023

Not sure if you need any changes in the task code as well. If they use queries then they probably need a filter by task domain.

Almost certainly - it's next on The List...

@ggainey ggainey force-pushed the 2920_domain_work branch from 690ece2 to 8c47431 Compare April 5, 2023 20:44
@ggainey
Copy link
Contributor Author

ggainey commented Apr 17, 2023

Tests that check content-access will fail until pulp/pulpcore#3748 gets merged.

@ggainey ggainey closed this Apr 17, 2023
@ggainey ggainey reopened this Apr 17, 2023
@ggainey ggainey force-pushed the 2920_domain_work branch 5 times, most recently from 709da07 to 370a3cd Compare April 19, 2023 19:41
@ggainey ggainey requested review from gerrod3, dralley and lubosmj April 19, 2023 20:26
@ggainey
Copy link
Contributor Author

ggainey commented Apr 19, 2023

OK folks - for the moment this is still in draft, but I need eyes, because I've looked at these files for Too Long. The tests all pass in both domain-on and domain-off mode. Improvements for the tests/fixtures very gratefully accepted, I based what I'm doing on pulp_file's work, but RPM is Different (and so is my brain).

@gerrod3 I am Absurdly Positive I have missed something Important - let me know if you think of/see anything.

@dralley
Copy link
Contributor

dralley commented Apr 20, 2023

Looking reasonable so far, I'll take a deeper dive tomorrow

@ipanova ipanova self-requested a review April 20, 2023 15:36
gerrod3
gerrod3 requested changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@gerrod3 gerrod3 left a comment

Choose a reason for hiding this comment

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

Looking good, this is close to being ready. What do you think about for adding docs?

pulp_rpm/app/models/package.py Outdated Show resolved Hide resolved
pulp_rpm/app/serializers/advisory.py Outdated Show resolved Hide resolved
pulp_rpm/app/serializers/custom_metadata.py Outdated Show resolved Hide resolved
pulp_rpm/app/serializers/package.py Outdated Show resolved Hide resolved
pulp_rpm/app/tasks/comps.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/api/test_domains.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/conftest.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/conftest.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/conftest.py Show resolved Hide resolved
pulp_rpm/tests/functional/conftest.py Outdated Show resolved Hide resolved
@ggainey ggainey marked this pull request as ready for review April 23, 2023 12:25
@gerrod3
Copy link
Contributor

gerrod3 commented Apr 26, 2023

@ggainey Have you gone over this checklist: https://docs.pulpproject.org/pulpcore/plugin_dev/plugin-writer/concepts/domains/domains_compatibility.html

I think some of the task code might still need to be looked over and more importantly you need to update the template_config.yml to activate domain testing in the CI.

@ggainey ggainey reopened this May 10, 2023
@ggainey ggainey force-pushed the 2920_domain_work branch from ab05715 to 35269f8 Compare May 12, 2023 19:42
@ggainey ggainey closed this May 15, 2023
@ggainey ggainey reopened this May 15, 2023
@ggainey ggainey closed this May 15, 2023
@ggainey ggainey reopened this May 15, 2023
@ggainey ggainey force-pushed the 2920_domain_work branch 2 times, most recently from dd9aa88 to 9ef3dc7 Compare May 15, 2023 15:33
@ggainey ggainey closed this May 15, 2023
@ggainey ggainey reopened this May 15, 2023
@ggainey ggainey force-pushed the 2920_domain_work branch from 9ef3dc7 to 796f216 Compare May 15, 2023 16:58
gerrod3
gerrod3 requested changes May 15, 2023
View reviewed changes
Copy link
Contributor

@gerrod3 gerrod3 left a comment

Choose a reason for hiding this comment

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

Implementation looks good! Some questions on the tests.

pulp_rpm/tests/functional/api/test_domains.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/api/test_domains.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/api/test_domains.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/api/test_domains.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/api/test_domains.py Outdated Show resolved Hide resolved
pulp_rpm/tests/functional/api/test_domains.py Show resolved Hide resolved
pulp_rpm/tests/functional/api/test_domains.py Show resolved Hide resolved
@ggainey ggainey force-pushed the 2920_domain_work branch 4 times, most recently from c6c50c1 to b43108a Compare May 17, 2023 15:31
@pavelpicka @ggainey
Add Domain support.
85b44dc 
closes pulp#3008

(based on PR pulp#2920)
Co-authored by: Grant Gainey <ggainey@redhat.com>
@ggainey ggainey force-pushed the 2920_domain_work branch from b43108a to 85b44dc Compare May 17, 2023 16:39
@ggainey
Copy link
Contributor Author

ggainey commented May 17, 2023

pulp-cli test failures:
test_rpm_sync_publish fails because it does this:
expect_succ curl "$curl_opt" --head --fail "$PULP_BASE_URL/pulp/content/cli_test_rpm_distro/config.repo"
which fails in domain-enabled environs. It should be using the distribution's base-url (or possibly being domain-aware and inserting domain-name when needed, but that feels overly complex). Getting this test to pass requires a fix and a pulp-cli release, and I propose it shouldn't hold up this PR merging into pulp_rpm.

The acs failure, I can't yet explain. All the tests end with "success" (start here ), it feels like cleanup() is what's triggering a failure here. Thoughts anyone?

@gerrod3
Copy link
Contributor

gerrod3 commented May 17, 2023

The acs failure, I can't yet explain. All the tests end with "success" (start here ), it feels like cleanup() is what's triggering a failure here. Thoughts anyone?

The acs test fails due to the grep -E expression not matching since domain is now in the url: https://github.com/pulp/pulp-cli/blob/main/tests/scripts/pulp_rpm/test_acs.sh#L41. Going to have to add some checks for domains or use a more lenient expression like pulp_file: https://github.com/pulp/pulp-cli/blob/main/tests/scripts/pulp_file/test_acs.sh#L41

@ggainey
Copy link
Contributor Author

ggainey commented May 17, 2023

The acs failure, I can't yet explain. All the tests end with "success" (start here ), it feels like cleanup() is what's triggering a failure here. Thoughts anyone?

The acs test fails due to the grep -E expression not matching since domain is now in the url: https://github.com/pulp/pulp-cli/blob/main/tests/scripts/pulp_rpm/test_acs.sh#L41. Going to have to add some checks for domains or use a more lenient expression like pulp_file: https://github.com/pulp/pulp-cli/blob/main/tests/scripts/pulp_file/test_acs.sh#L41

Ah yes, exactly so, thanks @gerrod3 .

OK - so my proposal is, we allow these failures for this PR. I will work on fixing pulp-cli, which will need a release before pulp_rpm PRs will stop noticing these failures. As long as we recognize that they're test-bugs, not rpm-domain-bugs, and are ok with seeing them for a few days, I think we can accept this PR.

Assuming nobody thinks of Something Else...

@ggainey
Copy link
Contributor Author

ggainey commented May 18, 2023

Test-failures are addressed in this PR

@ggainey
Copy link
Contributor Author

ggainey commented May 22, 2023

With the release of pulp-cli/0.19.2, the test failures were addressed and this PR is now green. Can has merge please? kthxbai

@dralley dralley merged commit 53636c0 into pulp:main May 22, 2023
@ggainey ggainey deleted the 2920_domain_work branch July 18, 2023 12:58
This was referenced Aug 8, 2023
Open
Open
@lubosmj lubosmj mentioned this pull request Mar 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdellweg mdellweg mdellweg left review comments

@gerrod3 gerrod3 gerrod3 requested changes

@dralley dralley dralley approved these changes

@lubosmj lubosmj Awaiting requested review from lubosmj

@ipanova ipanova Awaiting requested review from ipanova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

As a user, I have domain support
5 participants
@ggainey @dralley @gerrod3 @mdellweg @pavelpicka