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

Bring Wendy upto date with the new Bob #16

Merged
merged 17 commits into from
Dec 27, 2020
Merged

Bring Wendy upto date with the new Bob #16

merged 17 commits into from
Dec 27, 2020

Conversation

TimoKramer
Copy link
Member

@TimoKramer TimoKramer commented Dec 20, 2020

You can use the CLI so far. I did not yet work on the receiving of a tar file. And the api-spec request fails as well because of the header. You can read in data for a post request from file or via stdin on linux so far only.

clj -m wendy.main pipeline-create --data docs/build-simple.json --group dev --name test
cat docs/build-simple.json | clj -m wendy.main pipeline-create --data /dev/stdin --group dev --name test

Next Todos are:

  • impl PipelineArtifactFetch
  • remove api-spec from cli
  • test native-image compilation
  • commands are cut when calling --help
  • E2E-testing?
  • caching locally

@TimoKramer TimoKramer changed the title Thin client Bring Wendy upto date with the new Bob Dec 20, 2020
@TimoKramer TimoKramer self-assigned this Dec 20, 2020
@lispyclouds
Copy link
Member

lispyclouds commented Dec 20, 2020

This looks great, one observation: using /dev/stdin isn't cross platform and we should use something more generic like - and when thats is present use Java's System/in to use java's abstractions over the OS specific stdin. The - is a UNIX convention for stdin.

@lispyclouds
Copy link
Member

lispyclouds commented Dec 20, 2020

Or just assume that the data comes from stdin and for a thing needing to do a POST. If stdin is empty simply fail. This should be simpler? The same way HTTPie assumes input is on stdin when doing a POST instead of --data like cURL to reduce parsing and knowledge complexity. The issues arise from certain chars in the --data that could be specially understood by the invoking shell and fail in weird ways. Piping through stdin is simpler hence safer i think.

@TimoKramer
Copy link
Member Author

That is what I investigated of course but then you would need to hack around with cli-matic. I did not come up with a beautiful solution that offers multiplatform support. The presets are here: https://github.com/l3nz/cli-matic#current-pre-sets

So the way to go would be to go around cli-matic and just read from in when there is a body? I could try that 🤔

@TimoKramer
Copy link
Member Author

I thought about this some more. Httpie is not a perfect example for this because httpie doesn't tell you how to do a POST request. We on the other hand are guiding the user on how to do a pipeline-create command. That means there should be the help saying that the user needs to provide additional data as either --data file or stdin.

That means the optimal solution from UX perspective would be to have --data - expanding to stdin and providing a file path behaves the same. And at the same time the --data option would be mandatory. Best would be to have a :stdin pre-set for cli-matic.

It would be fine though to only use a pre-set :stdin then and not using files at all.

@lispyclouds
Copy link
Member

Totally agree, for simplicty reasons and opting to use the OS features more than us writing some code and being a bit future proof, I am of the opinion that lets drop files all together and just use stdin. Of course, we need a proper help text and some examples to show how to pipe the contents on various platforms. My thought process is if we introduce files and its accompanying parsing logic, we are introducing another layer and not using the OS/UNIX native and simpler piping/composing infra. I would say lets not use the files at all and if at all the need comes we can think of it then. What say?

@lispyclouds
Copy link
Member

I am also thinking in terms of wendy being in an automated env wherein having only files may be a complexity like downloading them, having a right path and/or permissions. simply using stdin address all of those and decomplects wendy from knowing how to read and its associated incedental complexity.

@TimoKramer
Copy link
Member Author

Totally agree, as I said using only stdin is totally cool. But the question is how. We would need to investigate contributing to cli-matic here.

@lispyclouds
Copy link
Member

That can be an option, for now can we manually check if the method is post and it needs a body, we fail internally, after cli-matic's parsing? Not all POSTs need a body like pipeline-start. We do need to write a helper fn that does this and if this is generic enough can be contributed upstream too? Lets not add the post body as a cli matic option for now and handle it internally when we are about to make the request.

@TimoKramer
Copy link
Member Author

This is the stdin issue open for cli-matic l3nz/cli-matic#28

@lispyclouds
Copy link
Member

Yeah so lets not worry about cli-matic's magical handling of the cli for now. Here is what I think can be done:

  • We read the YAML
  • For all the POST paths, we translate all the path and query params to cli-matic's args
  • Let cli-matic verify the stuff and call our handler with the params
  • Here we check if the path needed a post body, we check stdin, if nothing we fail, or we read and make the call

Hows that for starters?

src/wendy/request.clj Outdated Show resolved Hide resolved
src/wendy/request.clj Outdated Show resolved Hide resolved
@TimoKramer
Copy link
Member Author

I think this is good to merge for now.

@lispyclouds
Copy link
Member

Does this work with clojure 1.10.1 on Graal? I think cli-matic needed the 1.10.2 versions with the spec fix?

@TimoKramer
Copy link
Member Author

It worked when I tried it. Need to check my versions though. There was an error message I think, but it worked.

@lispyclouds
Copy link
Member

Check if it says A JVM will be needed to run this or something similar. If that's the case its not a true native image, it needs a JVM to figure out the dynamic stuff

@TimoKramer TimoKramer merged commit 3a2abc4 into main Dec 27, 2020
@TimoKramer TimoKramer deleted the thin-client branch December 27, 2020 11:51
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