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

Add support for passing json values to arguments as .txt files #84

Merged
merged 15 commits into from May 10, 2018

Conversation

Christina-Kang
Copy link
Contributor

Changes include:

  • Give users the option of passing in a .txt file where they have to pass in a json value for an argument.

Verifed:

  • Build and test CI passes
  • History and readme updated to reflect changes
  • Package version updated according to semantic versioning rules
  • Tests modified or added, when applicable
  • Updated code owners file, when applicable
  • Read the PR checklist

@coveralls
Copy link

coveralls commented Apr 12, 2018

Coverage Status

Coverage increased (+0.7%) to 80.533% when pulling 4125254 on Christina-Kang:accept-json-file into e507825 on Azure:master.

@Christina-Kang
Copy link
Contributor Author

@samedder Can you review this? Thanks :)

@samedder
Copy link
Collaborator

Is this something that Knack cannot support?

@Christina-Kang
Copy link
Contributor Author

@samedder I don't see support for this in their code or docs.

@samedder
Copy link
Collaborator

Rather than us writing custom logic for this, we should contribute to Knack or request them to support this. I don't think this is a feature that should only be added to our CLI.

@Christina-Kang
Copy link
Contributor Author

I've submitted an issue on their github, but that will take quite a bit of time I suspect. I do not have the time to add to their repo any time soon, and our commands are getting more unmanageable as we add more things for which we can't avoid json easily. We can then remove this when the knack team adds in that feature.

@samedder
Copy link
Collaborator

samedder commented May 2, 2018

If you've written all the code already for our CLI how much more effort would it be to migrate it back out to knack?

@Christina-Kang
Copy link
Contributor Author

@samedder I would need to go through all their code and determine what is the best way of adding this without impacting existing items. Probably as a pre-defined built in type, then understand their testing framework which I have not looked at, then update docs, etc. I would need to discuss with their team if that's how they want it to be implemented. It's a non-trivial amount of work to port this. I can add a tracking item and get to it when I have time, but I doubt it will be this month.

@samedder
Copy link
Collaborator

samedder commented May 3, 2018

@Christina-Kang two things:
1 - This sounds like it needs to be ported from Azure CLI, I'm fine merging this but we should push back and ask Knack to support the same feature set as Azure CLI
2 - Follow the same @ convention to specify file names

samedder
samedder previously approved these changes May 10, 2018
Copy link
Collaborator

@samedder samedder left a comment

Choose a reason for hiding this comment

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

Good to merge, but like I said be careful about adding features like this

@Christina-Kang Christina-Kang merged commit b14ca92 into microsoft:master May 10, 2018
jkochhar pushed a commit to jkochhar/service-fabric-cli that referenced this pull request May 30, 2018
…soft#84)

Add support for inputting JSON values by providing a path to a file rather than only by provided a JSON string. 

To do so, set argument value to the relative or absolute path of the text file prefixed by "@".
@Christina-Kang Christina-Kang deleted the accept-json-file branch November 1, 2018 21:25
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.

None yet

3 participants