You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository has been archived by the owner on May 3, 2022. It is now read-only.
Currently, each command seems to load bundles their own way. This means that each command is on its own to validate whether the bundle adheres to the spec. This affects users of duffle because they do not get clear, prompt feedback on their bundle.
For example, if a user runs duffle install myclaim -f /path/to/bundle -p params on a bundle where one of the destination keys has been misspelled, then they will observe that their bundle installs but that a value they are setting is not reaching the invocation image. Duffle will not provide any error feedback.
I propose that:
(1) All commands are updated to use a single helper function to load bundles from files -- i.e. loadBundleFromFile(pathToBundle string) (Bundle, error). This helper function should use the existing validation function. This is key to having all commands load bundles in the same way so that we can avoid inconsistent behavior.
(2) The above loadBundleFromFile function should completely validate the bundle against the CNAB spec. The spec literally includes a json schema for a bundle. Duffle should compare each bundle to the json schema to ensure that it matches. This is the key to providing thorough, immediate feedback to users.
(3) The code to load bundles by name from ~/.duffle/bundles should adhere to the storage interface, as claim.Store does. This would make it easier to follow code that needs to deal with loading bundles by name. When loading bundles by name from the ~/.duffle/bundles directory, this bundle.Store code should still use the loadBundleFromFile function above, so that it gets the same bundle validation.
I wonder if we could also guarantee that the only way to construct a bundle from a file is via a validating helper function as described above? For example, we could add a private field to the Bundle struct and use that to force the use of constructor functions in the same package to create instances of the struct. This may not be possible, but I think it would be worth a shot.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Currently, each command seems to load bundles their own way. This means that each command is on its own to validate whether the bundle adheres to the spec. This affects users of duffle because they do not get clear, prompt feedback on their bundle.
For example, if a user runs
duffle install myclaim -f /path/to/bundle -p params
on a bundle where one of thedestination
keys has been misspelled, then they will observe that their bundle installs but that a value they are setting is not reaching the invocation image. Duffle will not provide any error feedback.I propose that:
(1) All commands are updated to use a single helper function to load bundles from files -- i.e.
loadBundleFromFile(pathToBundle string) (Bundle, error)
. This helper function should use the existing validation function. This is key to having all commands load bundles in the same way so that we can avoid inconsistent behavior.(2) The above
loadBundleFromFile
function should completely validate the bundle against the CNAB spec. The spec literally includes a json schema for a bundle. Duffle should compare each bundle to the json schema to ensure that it matches. This is the key to providing thorough, immediate feedback to users.(3) The code to load bundles by name from
~/.duffle/bundles
should adhere to thestorage
interface, asclaim.Store
does. This would make it easier to follow code that needs to deal with loading bundles by name. When loading bundles by name from the~/.duffle/bundles
directory, thisbundle.Store
code should still use theloadBundleFromFile
function above, so that it gets the same bundle validation.Does this sound reasonable?
cc @youreddy
The text was updated successfully, but these errors were encountered: