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

Dynamic generated CLI #112

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dimon222
Copy link
Collaborator

@dimon222 dimon222 commented Nov 22, 2021

Fix #104

As there's just way too many methods to describe, I chose the least effortful approach to have maximum coverage.

New logic is going to pull the entire list of all available arguments in each of methods of each implemented class and generate dynamic argparsers on the fly, this way reducing headache of support in the future.
If any new method gets added, it can be included in the listing in main.py. If any method gets a different set of arguments, new logic will automatically use that.

Majority of devops engineers already know what all these arguments mean, and in terms of automation it will do just fine...

What is still pending:

  • Allow to pass main constructor arguments
  • Verify

@coveralls
Copy link

coveralls commented Nov 22, 2021

Pull Request Test Coverage Report for Build 3131333191

  • 22 of 27 (81.48%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+2.7%) to 92.063%

Changes Missing Coverage Covered Lines Changed/Added Lines %
yarn_api_client/main.py 22 27 81.48%
Files with Coverage Reduction New Missed Lines %
yarn_api_client/main.py 1 72.97%
Totals Coverage Status
Change from base Build 1682947314: 2.7%
Covered Lines: 290
Relevant Lines: 315

💛 - Coveralls

@dimon222 dimon222 marked this pull request as ready for review December 6, 2021 23:32
@dimon222
Copy link
Collaborator Author

dimon222 commented Dec 6, 2021

@kevin-bates I would appreciate second pair of eyes to look at it.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Hi Dimitry, this is really slick. I just had a couple of comments. Since I don't use the CLI, I'm not up on how this all comes into play. The changes look good and I trust you've checked things out - so I'm happy to approve this to move things forward. I'll leave the addressing of the comments up to you.

(Please note I'm on vacation through the end of the year, so my responses will be spotty. Thank you for all the work you do on this project!)

@@ -16,6 +17,10 @@ def get_parser():
description='Client for Hadoop® YARN API')

parser.add_argument('--endpoint', help='API endpoint (https://test.cluster.com:8090)')
#parser.add_argument('--api_class', help='Please provide api class - rm, hs, nm, am', required=True)
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentionally commented out? If so, perhaps an additional comment as to why or when it should be uncommented would be helpful.

yarn_api_client/main.py Outdated Show resolved Hide resolved
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.

CLI is out of date - methods and arguments
3 participants