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

Remove commands v1 from kadi #328

Merged
merged 4 commits into from
Apr 16, 2024
Merged

Remove commands v1 from kadi #328

merged 4 commits into from
Apr 16, 2024

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Apr 3, 2024

Description

This PR entirely removes commands v1 from kadi.

This includes some refactoring now that there is only one version of kadi commands.

The test coverage now in the no-internet case is improved with this PR. When there is no internet, kadi commands v2 get_cmds behaves the same as scenario="flight". In test_commands.py, previously many of the v2 tests were omitted for no internet. Now they are run using the flight archive only.

This PR supersedes #327.

Interface impacts

Commands v1 has been deprecated for a long time and now it is fully removed. The configuration setting commands_version is removed and the environment variable KADI_COMMANDS_VERSION no longer has any effect.

Testing

Unit tests

  • Mac
(ska3) ➜  kadi git:(remove-cmds-v1) pytest 
============================================= test session starts =============================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 178 items                                                                                           

kadi/commands/tests/test_commands.py .................................................................. [ 37%]
.............                                                                                           [ 44%]
kadi/commands/tests/test_states.py .......................x.......................                      [ 70%]
kadi/commands/tests/test_validate.py ....................                                               [ 82%]
kadi/tests/test_events.py ..........                                                                    [ 87%]
kadi/tests/test_occweb.py ......................                                                        [100%]

============================ 177 passed, 1 xfailed, 2 warnings in 74.43s (0:01:14) ============================

(ska3) ➜  kadi git:(remove-cmds-v1) git rev-parse HEAD   
355b284f7500790e90d755a09496a54db2db2c79

Independent check of unit tests by Jean

  • Linux
ska3-jeanconn-fido> pytest
====================================================== test session starts ======================================================
platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: anyio-4.3.0, timeout-2.2.0
collected 178 items                                                                                                             

kadi/commands/tests/test_commands.py ...............................................................................      [ 44%]
kadi/commands/tests/test_states.py .......................x.......................                                        [ 70%]
kadi/commands/tests/test_validate.py ....................                                                                 [ 82%]
kadi/tests/test_events.py ..........                                                                                      [ 87%]
kadi/tests/test_occweb.py ......................                                                                          [100%]

======================================================= warnings summary ========================================================
kadi/kadi/commands/tests/test_commands.py::test_get_starcats_each_year[year0]
  /proj/sot/ska3/flight/lib/python3.11/site-packages/setuptools_scm/git.py:308: UserWarning: git archive did not support describe output
    warnings.warn("git archive did not support describe output")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
===================================== 177 passed, 1 xfailed, 1 warning in 173.05s (0:02:53) =====================================
ska3-jeanconn-fido> git rev-parse HEAD
355b284f7500790e90d755a09496a54db2db2c79
ska3-jeanconn-fido> 

Functional tests

Also ran and passed all tests with no internet by turning off WiFi.

(ska3) ➜  kadi git:(remove-cmds-v1)  pytest                 
============================================= test session starts =============================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 178 items                                                                                           

kadi/commands/tests/test_commands.py .......sssssss..ss...............................................s [ 37%]
ss..s........                                                                                           [ 44%]
kadi/commands/tests/test_states.py .......................x.......................                      [ 70%]
kadi/commands/tests/test_validate.py .sssssssssssssssssss                                               [ 82%]
kadi/tests/test_events.py ..........                                                                    [ 87%]
kadi/tests/test_occweb.py ssssssssssssssssssssss                                                        [100%]

============================================== warnings summary ===============================================

@jeanconn
Copy link
Contributor

jeanconn commented Apr 3, 2024

As I noted in #327 (comment) I'm not sure if there is a use case to continue to skip the v2 tests if the machine doesn't have internet (sometimes a greta machine).

@taldcroft taldcroft requested a review from jeanconn April 3, 2024 19:27
@jeanconn
Copy link
Contributor

jeanconn commented Apr 5, 2024

I think there are still a few stray docs-things that were already outdated like

:class:`~kadi.commands.commands.CommandTable` of commands

And the reference still in

https://github.com/sot/kadi/blob/remove-cmds-v1/docs/api_commands.rst

I'm guessing from the way you've completely removed the v1 docs that removing them completely is the strategy you want, which seems fine.

@jeanconn jeanconn mentioned this pull request Apr 12, 2024
2 tasks
Copy link
Contributor

@jeanconn jeanconn 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 left a comment about some residual docs v1 cleanup, but that doesn't need to be a lien on this. My only other comment would be a quick question - should this code do something with KADI_COMMANDS_VERSION? I was wondering if it should in fact warn or error if set to not 2. But maybe that would have been more useful before we took away the v1 commands and is past moot now. Thanks!

@taldcroft taldcroft merged commit a588f6b into master Apr 16, 2024
3 checks passed
@taldcroft taldcroft deleted the remove-cmds-v1 branch April 16, 2024 13:19
This was referenced Apr 17, 2024
@javierggt javierggt mentioned this pull request May 1, 2024
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.

2 participants