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

feat: deprecation warnings on select application attributes #1116

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

dimaqq
Copy link
Contributor

@dimaqq dimaqq commented Sep 27, 2024

Description

  • explicitly export attributes accessible on Application via properties
  • mark some as deprecated, as these are unlikely to be available in Juju 4

juju/application.py Show resolved Hide resolved
juju/application.py Outdated Show resolved Hide resolved
tests/unit/test_application.py Outdated Show resolved Hide resolved
@dimaqq dimaqq marked this pull request as draft September 30, 2024 00:13
Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

I had a couple of questions that I accidentally left "pending" all weekend, but thankfully Ben asked them today. Do you think it would be worth describing anything about Application's properties in the class docstring?

@dimaqq dimaqq force-pushed the feat-application-deprecation branch 2 times, most recently from e4b9a8b to 1ec66d5 Compare September 30, 2024 08:51
@dimaqq
Copy link
Contributor Author

dimaqq commented Sep 30, 2024

This package already imports typing_extensions via some dependency.

I've made it an explicit dep, allowing for a simpler @deprecated decorator.

Here's a test run, showing correct source for the warning:

/code/python-libjuju/example.py:28: DeprecationWarning: Application.owner_tag is deprecated and will be removed in v4
  owner_tag.......... {app.owner_tag!r}
/code/python-libjuju/example.py:30: DeprecationWarning: Application.min_units is deprecated and will be removed in v4
  min_units.......... {app.min_units!r}
/code/python-libjuju/example.py:32: DeprecationWarning: Application.subordinate is deprecated and will be removed in v4
  subordinate........ {app.subordinate!r}
/code/python-libjuju/example.py:34: DeprecationWarning: Application.workload_version is deprecated and will be removed in v4, use Unit.workload_version instead.
  workload_version... {app.workload_version!r}

@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 1, 2024

I see doc changes as orthogonal, so maybe in a separate PR?

@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 1, 2024

CI failures are unrelated, #1108

@dimaqq dimaqq marked this pull request as ready for review October 1, 2024 04:50
Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

The @deprecated decorator is cool, didn't know about that. One or two result in rather long lines, but I think we can look at that later if we introduce linting with ruff/etc.

The CI failures are all known failing tests. 3/5 fail 100% of the time on main.
test_wait_for_idle_more_units_than_needed fails 80% of the time on main.
test_subordinate_false_field_exists fails around 25% of the time on main.

The changes made in this PR seem reasonable to me, but I would like the source of truth for the new property names and their type annotations to be mentioned somewhere, if not in a docstring then perhaps somewhere it will show up in the git log.

"""Class representing a juju application.

Explicit properties are annotated with data types inferred from ...
See ... juju documentation page?
"""

I think I want this because I don't know where they come from or why they're correct, and maybe a future reader will wonder the same thing. But if you don't think this is worthwhile here then let's skip it for now.

Also, maybe we want a <5 restriction on the typing_extensions dependency just to be conservative? I know the existing dependencies are loosely specified, but that already caused at least one CI breakage.

@dimaqq dimaqq force-pushed the feat-application-deprecation branch from 1ec66d5 to b359e0d Compare October 8, 2024 04:24
@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 8, 2024

The changes made in this PR seem reasonable to me, but I would like the source of truth for the new property names...

Done, ptal

@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 8, 2024

/build

juju/application.py Show resolved Hide resolved
@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 8, 2024

/merge

@dimaqq dimaqq force-pushed the feat-application-deprecation branch from 186ab56 to 5de27a9 Compare October 9, 2024 07:00
@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 9, 2024

/merge

@jujubot jujubot merged commit 1a44592 into juju:main Oct 9, 2024
10 of 12 checks passed
@dimaqq dimaqq deleted the feat-application-deprecation branch October 9, 2024 07:24
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.

4 participants