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

add pulp_timeout #164

Merged
merged 2 commits into from
Jul 5, 2024
Merged

Conversation

maciej-markowski
Copy link
Contributor

#163

this PR adds pulp_timeout parameter.

@@ -42,6 +42,11 @@ class ModuleDocFragment(object):
- It is recommended to use this once with the M(pulp.squeezer.status) module at the beginning of the playbook.
type: bool
default: false
pulp_timeout:
description:
- timeout value for connecting to pulp api
Copy link
Member

Choose a reason for hiding this comment

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

This is not what timeout does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

time in seconds to wait for successful connection to pulp api - would that be a better description?

Copy link
Member

Choose a reason for hiding this comment

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

No, you are trying to make this parameter available to the not yet rewritten modules:
https://github.com/pulp/squeezer/blob/develop/plugins/doc_fragments/pulp.py#L63

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if I follow.
I suppose what you're saying is that you're rewriting modules to use pulp_glue which already has this timeout?
As far as I can see, at least the rpm related modules are still using this openapi.py and it lacks this setting, which causes me some issues that I described in #163.
Can you explain what I am supposed to do to get this merged?

Copy link
Member

@mdellweg mdellweg Jun 25, 2024

Choose a reason for hiding this comment

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

Yes, basically.
You should use the same name and the same description at the very least. So rewriting the modules eventually will not break this again.

mdellweg
mdellweg previously approved these changes Jun 25, 2024
@maciej-markowski
Copy link
Contributor Author

I see the pipeline failed due to ansible removing support for python3.10
Can we still merge it and release or do we need to address this issues here?

@mdellweg
Copy link
Member

Nah, it would be better to fix the ci first.
Would you mind bumping some python versions in the workflows? Please make it a separate PR.

@maciej-markowski
Copy link
Contributor Author

Nah, it would be better to fix the ci first. Would you mind bumping some python versions in the workflows? Please make it a separate PR.

#166

@mdellweg
Copy link
Member

mdellweg commented Jul 4, 2024

Please rebase this.

@maciej-markowski
Copy link
Contributor Author

Are we going to merge #166 first?

@mdellweg
Copy link
Member

mdellweg commented Jul 4, 2024

I don't think, we need that anymore. I only adds more test combinations. And I am quite happy that we are down to 4 combinations, and still covering all supported ansible versions.

@mdellweg mdellweg merged commit f837b0f into pulp:develop Jul 5, 2024
7 checks passed
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