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

CLI: Adding new command 'configuration' #741

Closed
wants to merge 3 commits into from

Conversation

SatyamMattoo
Copy link
Contributor

@SatyamMattoo SatyamMattoo commented Aug 7, 2024

Short Description

Added a new configuration command that retrieves and displays the configuration of a specified adaptor. The command supports flags to return the sample schema, full schema, or both.

Related issue

Fixes #711

Implementation Details

Added a new configuration command to fetch and display adaptor configurations.
The command accepts an adaptor name (with or without a version) and retrieves the full configuration from the CDN.
Constructs a sample configuration using required fields and their examples from the full schema.
Supports --schema , --sample and --both (default) flags to specify the desired output.

Command: openfn configuration @openfn/language-dhis2 -o tmp/output.json --schema

QA Notes

Unit tests added for the command options and the configuration handler function. Tested locally with various adaptors to ensure correctness.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added unit tests

@josephjclark
Copy link
Collaborator

Hi @SatyamMattoo, thank you for this but shouldn't you be spending your time on the DMP job writing service?

@SatyamMattoo
Copy link
Contributor Author

Hello @josephjclark,
Yes sir, I have been working on that as well. While adding the describe adaptor service to the gen_job service, I figured out this solution. It took a day to work through, and I thought it would be quite helpful.

Best regards,

@josephjclark
Copy link
Collaborator

hmm, Ok. Unfortunately this is a very low priority feature. I would have preferred your day had been spent on DMP.

Having had a quick glance I notice you're downloading definitions from jsdelivr. But the CLI already has the ability to download adaptor versions to a local repo. Rather than download the data each time, I think we should be using the local repo. We already have similar functionality with the docs command, which should probably be the template for what you're doing here.

At the very least we should be doing some caching of results here. The slower the service is, the less useful it is to users.

I'll come back and look at this when I have a bit more time.

@josephjclark
Copy link
Collaborator

Hi @SatyamMattoo - please don't maintain this PR for the time being. It is not worth your time.

I will look at it when I have time and give you feedback so that we can merge it in.

@SatyamMattoo
Copy link
Contributor Author

SatyamMattoo commented Sep 12, 2024

Hello @josephjclark, Sorry, I was just updating the main branch for my fork. Moving forward, I’ll make sure to branch off the main and work on separate branches as needed.

@josephjclark
Copy link
Collaborator

@SatyamMattoo No need to apologise! You've done nothing wrong.

If you want to keep a fork of kit in sync with kit, then you'll need to branch this CLI feature into a new branch and then just preserve main. Otherwise, any other work you do from your fork will include this CLI feature, making it much harder to merge.

If you're planing on making more contributions I'd appreciate it if you messaged me first. I really don't have much capacity over the next few weeks and my priority is to work out how to merge your DMP branch. I just want to make sure you spend your time effectively.

@SatyamMattoo
Copy link
Contributor Author

@josephjclark Sure, I’ll branch it off as suggested. For the DMP, if I can assist with anything related to the merge or the feature, I’d be happy to help. I have some free time and can focus on speeding up the process.

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: add a schema command which prints configuration schema
2 participants