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

feat: Support Schema type in the Launch form #110

Merged
merged 2 commits into from
Oct 22, 2020
Merged

Conversation

schottra
Copy link
Contributor

TL;DR

Adds Launch form support for the Schema input type. The Schema structure will be pulled from the Task or Workflow closure. The user only needs to provide a uri indicating the location of the schema to load.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Adding an input was fairly straightforward. Since the only input we need is a string value, I reused all of the Simple input code paths to render a single text field. Parsing a Literal to an input value just requires reading the uri field off of the incoming Literal value.
The complication is in converting the input string to the final Literal value. The type value of the resulting Literal needs to be the schema value from the type definition in the source Task/Workflow closure. But the code for doing the parsing only had access to our mapped InputType enum and the input's value. To address this, I extended the InputTypeDefinition interface to also include the source literalType taken from the Variable we read out of the closure. This allows the conversion code to copy over the necessary information. This also has the advantage of allowing nested types such as collections to just work, since they will also pass down the nested variable types.

The majority of the changes ended up being all of the unit test cases, which previously only passed an InputType and now must pass a full InputTypeDefinition

Tracking Issue

flyteorg/flyte#405

Follow-up issue

NA

@codecov-io
Copy link

Codecov Report

Merging #110 into master will increase coverage by 0.40%.
The diff coverage is 82.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
+ Coverage   70.04%   70.45%   +0.40%     
==========================================
  Files         383      384       +1     
  Lines        6337     6431      +94     
  Branches      997     1024      +27     
==========================================
+ Hits         4439     4531      +92     
- Misses       1898     1900       +2     
Impacted Files Coverage Δ
.../components/Launch/LaunchForm/LaunchFormInputs.tsx 84.21% <ø> (+2.15%) ⬆️
src/components/Launch/LaunchForm/SimpleInput.tsx 93.33% <0.00%> (-3.22%) ⬇️
src/components/Launch/LaunchForm/constants.ts 100.00% <ø> (ø)
src/components/Launch/LaunchForm/types.ts 100.00% <ø> (ø)
src/components/Launch/LaunchForm/utils.ts 82.75% <64.28%> (-0.58%) ⬇️
...onents/Launch/LaunchForm/inputHelpers/constants.ts 100.00% <100.00%> (ø)
...aunch/LaunchForm/inputHelpers/getHelperForInput.ts 100.00% <100.00%> (ø)
...omponents/Launch/LaunchForm/inputHelpers/schema.ts 100.00% <100.00%> (ø)
...s/Launch/LaunchForm/inputHelpers/test/testCases.ts 100.00% <100.00%> (ø)
...components/Launch/LaunchForm/inputHelpers/utils.ts 86.36% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0209f9...9ba2be4. Read the comment docs.

@service-github-lyft-semantic-release
Copy link
Contributor

🎉 This PR is included in version 0.16.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@schottra schottra deleted the support-schema-type branch December 4, 2020 20:34
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.

4 participants