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

chore: refactor client_facades and specified_facades shape #1155

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

dimaqq
Copy link
Contributor

@dimaqq dimaqq commented Oct 10, 2024

Description

Streamlining facades code:

  • removing intermediate "versions" key from:
    • static facade version declaration
    • specified facades argument
  • deprecating the specified_facades argument as it's
    • not covered by unit or integration tests
    • not used by any library user

@james-garner-canonical
Copy link
Contributor

Thanks, I think it would be good to do something like this in future -- or even better, do away with the manually specified client_facades entirely. For now though I would like to avoid refactoring client_facades while we're still ironing out how to validate it and future schemas (3.4 series, 3.5 series, eventually 3.6). We can reopen this if we need to.

@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 11, 2024

reopening and moving back to draft; I'll rebase and update this after #1150

@dimaqq dimaqq reopened this Oct 11, 2024
@dimaqq dimaqq marked this pull request as draft October 11, 2024 03:37
@james-garner-canonical
Copy link
Contributor

Closing was probably premature, sorry.

I'd still prefer if we held off on merging this till we finish with #1099.

I know the changes in python-libjuju are pretty minor, but I've been using the current client_facades format in my scripts as the canonical representation for data generated from schemas, from _clientN.py files, from Juju's api/facadeversions.go, and of course client_facades itself. Not to mention introducing the need to handle both formats for client_facades when comparing across commits before and after this change.

All doable to fix, of course, but probably not worth having to fix -- unless this change simplifies work for you?

@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 11, 2024

totally fine to sit on this.

@dimaqq dimaqq force-pushed the chore-refactor-facade-versions branch from 607f28f to 8d3cad1 Compare October 17, 2024 02:23
@dimaqq dimaqq marked this pull request as ready for review October 17, 2024 02:36
@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 17, 2024

@james-garner-canonical rebased and updated the test, it's ready for review.

juju/client/connection.py Outdated Show resolved Hide resolved
juju/client/connection.py Show resolved Hide resolved
tests/validate/test_facades.py Outdated Show resolved Hide resolved
juju/client/connection.py Outdated Show resolved Hide resolved
juju/client/connection.py Outdated Show resolved Hide resolved
@dimaqq dimaqq force-pushed the chore-refactor-facade-versions branch from 31baac1 to b96cc1c Compare October 18, 2024 04:44
@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 18, 2024

/merge

@dimaqq dimaqq force-pushed the chore-refactor-facade-versions branch from b96cc1c to dfeb7fc Compare October 18, 2024 07:59
@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 18, 2024

/merge

1 similar comment
@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 22, 2024

/merge

@jujubot jujubot merged commit c58ec9d into juju:main Oct 22, 2024
22 of 23 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.

3 participants