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

Cast #915

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

Cast #915

wants to merge 2 commits into from

Conversation

mdellweg
Copy link
Member

Review Checklist:

  • An issue is properly linked. [feature and bugfix only]
  • Tests are present or not feasible.
  • Commits are split in a logical way (not historically).

@mdellweg mdellweg force-pushed the cast branch 2 times, most recently from c09fa4f to 00621cc Compare March 14, 2024 14:09
@mdellweg mdellweg marked this pull request as ready for review March 14, 2024 14:35
@mdellweg mdellweg enabled auto-merge (rebase) March 14, 2024 14:35
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.

Looks good, mainly have clarifying questions before approval.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remind me again how these named changelog entries work? Why not file an issue for them?

Copy link
Member Author

Choose a reason for hiding this comment

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

They will appear without as link.



class PulpTestContext(PulpContext):
# TODO check if we can just make the base class ignore echo.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean, doesn't the base class raise a NotImplemented error?

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 does, and that means, the PulpContext is unusable as is. I think that is a bad thing.

from pulp_glue.common.context import _REGISTERED_API_QUIRKS, PulpContext

pytestmark = pytest.mark.glue
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this another pytest quirk? How does it work?

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 marks all tests in this scope. (Putting it in conftest did not work sadly.)

Comment on lines 98 to 122
@pytest.fixture(scope="session")
def pulp_cli_settings_path(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice refactor!

Comment on lines +30 to +31
if hasattr(plugin, "mount"):
plugin.mount()
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently none of the pulp_glue plugins have a mount method, do you see the need for one in the future?

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'm not sure. This would be better "have" than "need".

@@ -33,6 +33,15 @@ documentation = "https://docs.pulpproject.org/pulp_cli/"
repository = "https://github.com/pulp/pulp-cli"
changelog = "https://docs.pulpproject.org/pulp_cli/CHANGES/"

[project.entry-points."pulp_glue.plugins"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you opened up the corresponding PRs in our other cli-plugin repositories? They won't be registered as loaded until they have made this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is right, but i think it should be safe to fade this in slowly.

@mdellweg mdellweg marked this pull request as draft March 18, 2024 11:29
auto-merge was automatically disabled March 18, 2024 11:29

Pull request was converted to draft

This allows to auto-detect and load all installed pulp-glue plugins. It
is primarily useful for workflows where knowing all sub-types of an
Entity is only important at runtime.
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.

2 participants