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

Makes CONTENT_ORIGIN not a required setting PULP-194 #6175

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

git-hyagi
Copy link
Contributor

No description provided.

docs/admin/reference/settings.md Outdated Show resolved Hide resolved
@git-hyagi git-hyagi force-pushed the content-origin-none-update branch from e5b2050 to cb4755c Compare January 7, 2025 12:47
pedro-psb
pedro-psb previously approved these changes Jan 7, 2025
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.

Please make sure one of the test scenarios is using None on the setting.

@bmbouter
Copy link
Member

bmbouter commented Jan 7, 2025

Please make sure one of the test scenarios is using None on the setting.

Can you really specifically outline what needs to happen here? Which test scenario and can you link to where that change would need to be made?

@pedro-psb
Copy link
Member

Scenarios are the environments for each test job in the CI (e.g, "pulp", "s3", "azure" and "lowerbound").
This configuration is done on .ci/ansible/settings.py.j2. In the selected range, you can see how to specify scenario-specific configurations with conditionals.

What I understand is that some (arbitrary) set of scenarios should set CONTENT_ORIGIN and some should not.
I would suggest s3 or azure, so we have a similar control pair.
Alternatively, a new scenario could be created, but that would be more costly and require plugin_template changes.

@bmbouter
Copy link
Member

bmbouter commented Jan 7, 2025

@pedro-psb this is really helpful, ty. This setting is defined earlier (L#1), and I'm not 100% sure what setting the same variable in two places in that would do. What do you think about us removing it from being set at L#1 and instead setting it on one or more of the other test runners?

Also, I don't know what you mean about "similar control pair", what does that mean.

@pedro-psb
Copy link
Member

pedro-psb commented Jan 7, 2025

Yeah, that sounds ideal, that shouldn't be set in the common settings part.

I was refering to the notion of experiment-control groups, where we have isolated differences being tested (and similar properties for everything else). But that was a bit of a loose reasoning, if something goes wrong it will probably be obvious in any of those scenarios.

@mdellweg
Copy link
Member

mdellweg commented Jan 8, 2025

This configuration is done on .ci/ansible/settings.py.j2. In the selected range, you can see how to specify scenario-specific configurations with conditionals.

To add more context the actual scenario_specific settings are set in the template_config.yml. Reapplying the template in the same commit is necessary.

pulp_settings_azure:

@bmbouter
Copy link
Member

bmbouter commented Jan 8, 2025

Thanks @mdellweg this is also helpful. Do you think we'll need to make any changes to the plugin template itself, or just update these and re-apply? Unforseen things can always come up, but I wanted to hear what you thought we'd run into.

@mdellweg
Copy link
Member

mdellweg commented Jan 9, 2025

No template changes. That's what we have these variables for.

@bmbouter
Copy link
Member

@git-hyagi I think this ticket has enough info on it for you to proceed making the variable changes and re-applying the template. Please come back with any questions at all. Thank you!

@git-hyagi git-hyagi force-pushed the content-origin-none-update branch from 9129d11 to 4829da1 Compare January 13, 2025 11:59
@git-hyagi git-hyagi marked this pull request as draft January 13, 2025 12:14
@mdellweg mdellweg dismissed their stale review January 13, 2025 13:08

Setting was changed.

git-hyagi added a commit to git-hyagi/pulp-cli that referenced this pull request Jan 17, 2025
If `content_origin` is not set, the `distribution.base_url` will be
defined with the relative path and the curl command in the test
script will fail to run because of the missing hostname/address.

ref: pulp/pulpcore#6175
@git-hyagi git-hyagi force-pushed the content-origin-none-update branch from 4829da1 to 3f8be97 Compare January 17, 2025 20:18
@git-hyagi git-hyagi marked this pull request as ready for review January 20, 2025 13:58
@git-hyagi
Copy link
Contributor Author

The CI is probably failing because we depend on a change in pulp-cli tests: pulp/pulp-cli#1118

ggainey pushed a commit to pulp/pulp-cli that referenced this pull request Jan 20, 2025
If `content_origin` is not set, the `distribution.base_url` will be
defined with the relative path and the curl command in the test
script will fail to run because of the missing hostname/address.

ref: pulp/pulpcore#6175
patchback bot pushed a commit to pulp/pulp-cli that referenced this pull request Jan 20, 2025
If `content_origin` is not set, the `distribution.base_url` will be
defined with the relative path and the curl command in the test
script will fail to run because of the missing hostname/address.

ref: pulp/pulpcore#6175
(cherry picked from commit 466f1eb)
ggainey pushed a commit to pulp/pulp-cli that referenced this pull request Jan 20, 2025
If `content_origin` is not set, the `distribution.base_url` will be
defined with the relative path and the curl command in the test
script will fail to run because of the missing hostname/address.

ref: pulp/pulpcore#6175
(cherry picked from commit 466f1eb)
@git-hyagi git-hyagi force-pushed the content-origin-none-update branch 3 times, most recently from fa1f7e7 to d7dc98b Compare January 20, 2025 19:25
@git-hyagi git-hyagi requested review from dkliban and ggainey January 21, 2025 00:10
pulpcore/app/serializers/fields.py Show resolved Hide resolved
pulpcore/app/settings.py Outdated Show resolved Hide resolved
@@ -58,6 +58,7 @@ pulp_settings:
tmpfile_protection_time: 10
upload_protection_time: 10
pulp_settings_azure:
content_origin: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@git-hyagi git-hyagi force-pushed the content-origin-none-update branch from a2b5635 to a1e1b61 Compare January 21, 2025 10:35
@mdellweg mdellweg linked an issue Jan 21, 2025 that may be closed by this pull request
dkliban
dkliban previously approved these changes Jan 21, 2025
pulp_file/pytest_plugin.py Outdated Show resolved Hide resolved
pulpcore/app/models/publication.py Show resolved Hide resolved
pulpcore/app/serializers/replica.py Outdated Show resolved Hide resolved
pulpcore/app/replica.py Outdated Show resolved Hide resolved
pulpcore/app/serializers/status.py Outdated Show resolved Hide resolved
@git-hyagi git-hyagi force-pushed the content-origin-none-update branch from e81499a to 09aeded Compare January 21, 2025 19:28
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.

Pulp needs to support multiple gateways with different FQDN
7 participants