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

Autocompletion of ioc and version #60

Merged
merged 24 commits into from
Dec 8, 2023

Conversation

marcelldls
Copy link
Collaborator

Possibly resolves #46 - have only applied to deploy command

@marcelldls
Copy link
Collaborator Author

tiangolo/typer#334

@marcelldls
Copy link
Collaborator Author

The performance is not amazing - some caching would help speed up subsequent calls

Copy link
Member

@gilesknap gilesknap left a comment

Choose a reason for hiding this comment

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

This looks really promising. One other thing we need completion on is files in the file system.
e.g. when you type:
ec ioc deploy-local iocs/ tab tab

I had hoped that typer would have built in support for this but I did not find it when I looked, so you may have to resort to implementing your own using PathLib.

tag
"""
check_beamline_repo(beamline_repo)
typer.echo(f"Available instance versions for {ioc_name}:")

run_command(f"git clone {beamline_repo} {folder}", interactive=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

shallow clone would probably help here

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, as this is a cli, we could lazily clone this the first time we need it, grab the info out of it and store it as a global, then use it multiple times

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could make it a pure function with no side effects (i.e. clone, get instance and version info, delete, then return info) and cache it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe we need the full history to get all the available tags for each IOC so I don't believe shallow clone is an option. Im not sure the global works as each auto complete is a new run of the code, or is there a nice way to have persistent variables i am not aware of? Giles and I did discuss creating a graph of IOCs and versions once and just sharing it as you have mentioned. I have an attempt at pure functions like you have descibed and I think it works quite well but I will try a graph approach as well

Copy link
Member

@GDYendell GDYendell Nov 30, 2023

Choose a reason for hiding this comment

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

If you only need to list the tags, what about git ls-remote --tags <repo>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its not just listing the tags - there is a relationship between the tags and last changes to the IOC

@gilesknap
Copy link
Member

@marcelldls this looks really good. I have tried it and it works well.

Things still to do (you probably already know this but let's make sure we are on the same page):

  • add ioc name completion to these ioc subcommands using kubectl to query which are available. This one is a bit challenging because we need to use the "local_commands" instead of "k8s_commands" approach if the EC_K8S_DOMAIN==local (you could ignore this for now as it's only for non dls-users and the tutorials):
    • attach
    • delete
    • exec
    • log-history
    • logs
    • restart
    • start
    • stop
    • template
  • add filename completion to
    • deploy-local
    • validate
    • template
  • add version completion to deploy when you have specified a local folder instead of a name (I think that is correct - maybe we could use the version from the yaml file if none is supplied but is that assuming too much?)

@marcelldls
Copy link
Collaborator Author

This looks really promising. One other thing we need completion on is files in the file system. e.g. when you type: ec ioc deploy-local iocs/ tab tab

I had hoped that typer would have built in support for this but I did not find it when I looked, so you may have to resort to implementing your own using PathLib.

tiangolo/typer#663

@GDYendell
Copy link
Member

tiangolo/typer#663

From that issue:

The workaround I found is to set the type to str instead of Path but I'm not satisfied with it.

That can't be right surely? If you use str it autocompletes files from the current directory?

@marcelldls
Copy link
Collaborator Author

tiangolo/typer#663

From that issue:

The workaround I found is to set the type to str instead of Path but I'm not satisfied with it.

That can't be right surely? If you use str it autocompletes files from the current directory?

I believe if it expects a string but you dont give it a list of strings to use for auto complete it reverts to some behavior which is a path autocompletion

I think this is a click issue which is broken for Path type arguments - which should be giving nice path autocompletion. The problem with the work around to use str arguments is that you dont get the built in path validation. I have found my own work around which is to give an autocompletion function which returns [] - this seems to force path autocompletion and still retain the path validation

@gilesknap
Copy link
Member

have found my own work around which is to give an autocompletion function which returns [] - this seems to force path autocompletion and still retain the path validation

Let's make sure we get a minimum reproduction of this issue reported to typer, please.

@gilesknap
Copy link
Member

Just trying this out and it looks freakin awesome!

Some pedantry:

  • ec ioc start shows already started iocs (but starting an already started IOC is benign)
  • ec ioc logs lists all ioc deployments but it hangs when logging a non-running deployment

I just checked and kubectl -n i20-1-iocs logs deploy/bl20j-di-phdgn-05 does time out if there is no pod running. Probably best to only show running IOCS for 'logs' . This is probably expected behaviour - deployments with single pods can log that pod - deployments with no pods to log probably wait for one to come up.

Copy link
Member

@gilesknap gilesknap left a comment

Choose a reason for hiding this comment

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

I'm happy with this now.

I feel like we could go ahead and merge, then add some unit tests when you return from vacation.

@marcelldls marcelldls marked this pull request as ready for review December 7, 2023 16:41
@gilesknap
Copy link
Member

@marcelldls much better error handling.

One niggle:

(ec-venv) (python3.11) (ioc-autocomplete) [hgv27681@pc0116 epics-containers-cli]$ ec ioc exec Please set EC_K8S_NAMESPACE or pass --namespace
Please set EC_K8S_NAMESPACE or pass --namespace

docs/                    get_helm.sh              .github/                 LICENSE                  pyproject.toml           src/                     .vscode/                 
environment.sh           .git/                    .gitignore               .pre-commit-config.yaml  README.rst               tests/                   
(ec-venv) (python3.11) (ioc-autocomplete) [hgv27681@pc0116 epics-containers-cli]$ ec ioc exec 

We still get the file completion after printing the error message, which could be confusing.

@gilesknap
Copy link
Member

gilesknap commented Dec 8, 2023

Something is horribly wrong:

(ec-venv) (python3.11) (dev) [hgv27681@pc0116 bl20j]$ ec ioc deploy-local iocs/bl20j-
bl20j-di-phdgn-02/ bl20j-di-phdgn-03/ bl20j-di-phdgn-05/ bl20j-di-phdgn-07/ bl20j-di-phdgn-08/ bl20j-mo-brick-06/ 
(ec-venv) (python3.11) (dev) [hgv27681@pc0116 bl20j]$ ec ioc deploy bl20j-
bl20j-di-ioc-01    bl20j-ea-phdgn-02  bl20j-ea-phdgn-03  bl20j-ea-phdgn-05  bl20j-ea-phdgn-07  bl20j-ea-phdgn-08  

What I have in my iocs folder and what is available to deploy disagree - this is probably my issue rather than yours.

@marcelldls
Copy link
Collaborator Author

@marcelldls much better error handling.

One niggle:

(ec-venv) (python3.11) (ioc-autocomplete) [hgv27681@pc0116 epics-containers-cli]$ ec ioc exec Please set EC_K8S_NAMESPACE or pass --namespace
Please set EC_K8S_NAMESPACE or pass --namespace

docs/                    get_helm.sh              .github/                 LICENSE                  pyproject.toml           src/                     .vscode/                 
environment.sh           .git/                    .gitignore               .pre-commit-config.yaml  README.rst               tests/                   
(ec-venv) (python3.11) (ioc-autocomplete) [hgv27681@pc0116 epics-containers-cli]$ ec ioc exec 

We still get the file completion after printing the error message, which could be confusing.

Fair enough. A difficulty is if you dont want that you have to give something to autocomplete...
I do think I have a better approach now though to rather get

(ec-dev) (base) vboxuser@Ubuntu-dev:~/WIP/epics-containers-cli$ ec ioc attach Please set EC_K8S_NAMESPACE or pass --namespace
  Please set EC_K8S_NAMESPACE or pass --namespace
  Please set EC_K8S_NAMESPACE or pass --namespace
  Please set EC_K8S_NAMESPACE or pass --namespace

But I dont really understand fully why it works

@gilesknap
Copy link
Member

(ec-dev) (base) vboxuser@Ubuntu-dev:~/WIP/epics-containers-cli$ ec ioc attach Please set EC_K8S_NAMESPACE or pass --namespace
  Please set EC_K8S_NAMESPACE or pass --namespace
  Please set EC_K8S_NAMESPACE or pass --namespace
  Please set EC_K8S_NAMESPACE or pass --namespace

Yep I think that is better.

@gilesknap
Copy link
Member

So I got my bl20j repo to look OK:

(ec-venv) (python3.11) (dev) [hgv27681@pc0116 bl20j]$ ec ioc deploy-local iocs/bl20j-
bl20j-di-phdgn-02/ bl20j-di-phdgn-03/ bl20j-di-phdgn-05/ bl20j-di-phdgn-07/ bl20j-di-phdgn-08/ bl20j-mo-brick-06/ 
(ec-venv) (python3.11) (dev) [hgv27681@pc0116 bl20j]$ ec ioc deploy bl20j-
bl20j-di-phdgn-02  bl20j-di-phdgn-03  bl20j-di-phdgn-05  bl20j-di-phdgn-07  bl20j-di-phdgn-08  bl20j-mo-brick-06  

The thing that I did was push my changes to main (from dev). It's not clear to me why this was needed. I have been putting my tags on the dev branch for that repo (and probably should not be doing that!). Perhaps I will allow working in the main branch for beamline repos anyway - anything else seems to make extra work.

@marcelldls marcelldls merged commit cbccded into epics-containers:main Dec 8, 2023
7 checks passed
gilesknap added a commit that referenced this pull request Dec 13, 2023
* More general ioc versions function

* Add autocomplete skeleton

* More skeleton code

* Implement autocomplete

* Linting

* Ioc autocomplete cached (#2)

* Attempt caching for autocomplete

* Create available IOC & version graph

* Add catch for version autocomplete

* Remove caching from ioc instances call

* Handle --repo option

* Implement unique cache per domain repo

* Ignore files present in iocs/

* Add workaround for path autocompletion

* Add kubectl autocompletions

* Change ec logs autocomplete to running iocs

* Add some typing + error handling for autocomplete

* Ignore typer deprecated warning

* Make mypy happier

* Linting

* Make autocomplete keyerror behavior consistant

* Autocomplete with error rather than files

* fix instances test

* Fix f-string for failed test

* Include directory context manager addition

---------

Co-authored-by: Giles Knap <giles.knap@diamond.ac.uk>
@marcelldls marcelldls deleted the ioc-autocomplete branch March 1, 2024 08:20
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.

improve cli command line completion
4 participants