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

Cleanup the traefik-v2 branch #134

Merged

Conversation

GeorgianaElena
Copy link
Member

This cleans up a bit the traefik-v2 branch so that we can better organise migrating the proxies to traefik v2.

It removes:

  • the proxy performance evaluation
  • the two key-value store proxies (etcd and consul) from the package

It also runs the tests just for the toml proxy, since this is the first one that we will be migrating to v2 of traefik.

Ref: #133 (comment)

@alexleach
Copy link
Contributor

Hi @GeorgianaElena , just catching up with this on my phone now. Personally, I quite like your idea of removing all the kv_proxy / consul and etcd proxy code. (Not sure if I saw you'd removed relevant code in install.py, too?).

It will certainly make the PR's smaller, and probably a lot tidier than just commenting out the relevant test fixture parameters in test_proxy.py and test_traefik_api_auth.py, which is what I've been doing.

One comment on this relating to the readthedocs failing. It's something to do with sphinx v4 removing PyClassmember. If you pull a new version of autodoc_traits.py, then it will build okay. I initially thought this was installed with pip, but then saw you've included a copy, in https://github.com/jupyterhub/traefik-proxy/tree/master/docs/sphinxext

Anyway, grab a new copy from here and the docs will build okay:-
https://github.com/jupyterhub/autodoc-traits/blob/main/autodoc_traits/autodoc_traits.py

Will try and figure out how to rebase my traefik-v2-file branch against this one later today.

Cheers,
Alex

@alexleach alexleach mentioned this pull request Jul 6, 2021
@GeorgianaElena GeorgianaElena changed the base branch from master to traefik-v2 July 7, 2021 10:30
@GeorgianaElena
Copy link
Member Author

Hi @GeorgianaElena , just catching up with this on my phone now. Personally, I quite like your idea of removing all the kv_proxy / consul and etcd proxy code. (Not sure if I saw you'd removed relevant code in install.py, too?).

I didn't actually removed the key-value store proxy files, but removed them from the module and tests. I figured keeping them will help with the diff when we switch those to v2 instead. Not sure if this is the best strategy, but until we decide if we do a release or not without them, there's no pressure 😅

Also, I didn't remove the code for installing etcd and consul from the installer. Instead, I removed the --etcd and --consul flags from the CI and don't install them anymore, which I think it's fine atm.

One comment on this relating to the readthedocs failing. It's something to do with sphinx v4 removing PyClassmember. If you pull a new version of autodoc_traits.py, then it will build okay. I initially thought this was installed with pip, but then saw you've included a copy, in https://github.com/jupyterhub/traefik-proxy/tree/master/docs/sphinxext

Anyway, grab a new copy from here and the docs will build okay:-
https://github.com/jupyterhub/autodoc-traits/blob/main/autodoc_traits/autodoc_traits.py

Thanks for pointing this out ☀️ I removed the autodoc_traits copy from this repo and installed it via pip instead, similar with what's in jupyterhub.

Will try and figure out how to rebase my traefik-v2-file branch against this one later today.

I will leave this PR open for today too and self-merge tomorrow if nobody has any objections to it. Afterwards, I think it'd be safe to rebase your branch (the one with the cherry-picked commits from the big one) against the main traefik-v2 branch of this repo and go from there :)

@alexleach
Copy link
Contributor

Sounds good, thanks. I won't touch anything for now and will get back to it later in the week then.
KR, Alex

@GeorgianaElena
Copy link
Member Author

Thanks @alexleach! I'm going to merge this now 🎉

@GeorgianaElena GeorgianaElena merged commit 3101914 into jupyterhub:traefik-v2 Jul 8, 2021
@alexleach
Copy link
Contributor

Just wanted to say I haven't forgotten or abandoned this, but have had to focus on some other projects for work the last couple of weeks. Hoping to get back to this next week though. Have a good weekend ☺️

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.

3 participants