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

Deprecations for v2 upgrades #154

Merged
merged 9 commits into from
Feb 22, 2023
Merged

Deprecations for v2 upgrades #154

merged 9 commits into from
Feb 22, 2023

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 21, 2023

Adds deprecated aliases for the APIs that were removed in #145

should prevent unnecessary breakage while folks upgrade

closes #149


Deprecations

TraefikConsulProxy class

  • kv_url is deprecated, use consul_url
  • kv_username is deprecated, use consul_username
  • kv_password is deprecated, use consul_password

TraefikEtcdProxy class

  • kv_url is deprecated, use etcd_url
  • kv_username is deprecated, use etcd_username
  • kv_password is deprecated, use etcd_password

TraefikTomlProxy class

  • The class is deprecated, use TraefikFileProviderProxy instead
  • toml_dynamic_config_file is deprecated, use dynamic_config_file instead. YAML is also supported.

TraefikProxy class (all classes)

  • toml_static_config_file is deprecated, use static_config_file. YAML is also supported.

avoid using generic `kv_` prefix for _not_ generic config like url, username, password
can sublass :class:`TKvProxy`.
"""

kv_client = Any()
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 removed these generic traits from KVProxy which weren't actually generic - these are unused by the base class and not actually general, even though our two subclasses have sufficiently similar setups.

I traded them for more descriptive and specific etcd_username, etc. config on the specific implementation classes, since these should be implementation-specific details, not generic features.

avoid unnecessary breakage by leaving old names available with deprecation warnings
Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

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

Thank you @minrk

docs/source/consul.md Outdated Show resolved Hide resolved
@GeorgianaElena
Copy link
Member

The etcd tests seem to have hanged as they run from over an hour and didn't finish :/

Screenshot 2023-02-21 at 18 56 19

@consideRatio
Copy link
Member

I cancelled the etcd workflow that kept running now, 4 hours in.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Besides having a test issue that needs to be addressed, this LGTM! I also figured its useful to have the deprecations listed in the PR description, so I added what I understand them to be so please review that edit I made also.

Resolve or understand the test hanging to be unrelated and this LGTM!

@minrk minrk merged commit 9b6bcef into jupyterhub:main Feb 22, 2023
@minrk minrk deleted the deprecations branch February 22, 2023 15:02
@minrk
Copy link
Member Author

minrk commented Feb 22, 2023

Thanks! Hang was the result of merging a few test changes, seems to be all good now.

@alexleach
Copy link
Contributor

There's still a run in progress actually, been going 5 hours and counting: https://github.com/jupyterhub/traefik-proxy/actions/runs/4241217403

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.

backward-compatibility for toml, etc.
4 participants