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.

Sign up for GitHub

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: inline Environment #403

Merged
merged 16 commits into from
Nov 26, 2020
Merged

feat: inline Environment #403

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
993c559
allow inline Environment
Duologic Oct 11, 2020
5c506a3
refactor: evalJsonnet -> eval
Duologic Oct 22, 2020
b18a386
refactor: use spec.Parse
Duologic Nov 4, 2020
ac417d3
refactor: extract Environment as k8s-like object
Duologic Nov 4, 2020
2ee1696
fix(cli): `tk env list` for inline envs
Duologic Nov 6, 2020
12ece72
fix: allow eval with err
Duologic Nov 18, 2020
943565f
s/v1alpha1.Config/v1alpha1.Environment
Duologic Nov 18, 2020
695e7a3
s/extract/extractedEnvs
Duologic Nov 18, 2020
b6ae81b
docs: update function descriptions
Duologic Nov 18, 2020
065a3e1
fix: return env names with error
Duologic Nov 20, 2020
f16c942
fix: log expected errors on eval
Duologic Nov 20, 2020
4ee8de7
fix: ensure metadata.name is set on loading an env
Duologic Nov 20, 2020
6533972
doc: remove out of place docstring
Duologic Nov 20, 2020
4f11ab2
fix: omitempty Data
Duologic Nov 20, 2020
9c30b46
conditional return values
Duologic Nov 26, 2020
52ba439
no switch inside if
Duologic Nov 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/tanka/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func parseEnv(path string, opts jsonnet.Opts, evalFn evaluateFunc) (interface{},
if _, ok := err.(process.ErrorPrimitiveReached); ok {
if specEnv == nil {
// if no environments or spec found, behave as jsonnet interpreter
return data, nil, err
return data, nil, ErrNoEnv{path}
}
} else if err != nil {
return nil, nil, err
Expand Down
3 changes: 1 addition & 2 deletions pkg/tanka/parse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"testing"

"github.com/grafana/tanka/pkg/jsonnet"
"github.com/grafana/tanka/pkg/process"
"github.com/grafana/tanka/pkg/spec/v1alpha1"
"github.com/stretchr/testify/assert"
)
Expand Down Expand Up @@ -117,7 +116,7 @@ func TestEvalJsonnet(t *testing.T) {
if data == nil {
assert.NoError(t, e)
} else if e != nil {
assert.IsType(t, process.ErrorPrimitiveReached{}, e)
assert.IsType(t, ErrNoEnv{}, e)
}
assert.Equal(t, test.expected, data)
assert.Equal(t, test.env, env)
Expand Down
15 changes: 14 additions & 1 deletion pkg/tanka/workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package tanka

import (
"fmt"
"log"

"github.com/fatih/color"

Expand Down Expand Up @@ -185,5 +186,17 @@ func Show(baseDir string, opts Opts) (manifest.List, error) {
// Eval returns the raw evaluated Jsonnet output (without any transformations)
func Eval(dir string, opts Opts) (raw interface{}, err error) {
r, _, err := eval(dir, opts.JsonnetOpts)
return r, err
if err != nil {
switch err.(type) {
case ErrMultipleEnvs:
log.Println(err)
Copy link
Member

Choose a reason for hiding this comment

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

I must admit I can't quite follow this code. To me this seems as encountering multiple environments is no longer a fatal thing, but only a warning, given no error is returned.

I'd expect Eval to behave like so:

  • return an error everytime an error occured (e.g. invalid Jsonnet, no Environment, multiple Environments, etc)
  • as you need to handle the "no inline but maybe spec.json" case further down the road, this specific case should also return data

This allows a naive callee to automatically abort when any error occurs. Callees aware of inline vs spec.json can react to receiving ErrNoEnv

Copy link
Member Author

Choose a reason for hiding this comment

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

That is what I did before, simply return both data and err from upstream to downstream, no handling here.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but this is different from what I just described.

Only the specific case of "no inline environment" has a reason to return data alongside an error. Afaict, all other errors that occur here are fatal and thus return nil, err

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so here you say return data+err? Gotcha.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, which you could simplify along those lines:

// Eval returns the raw evaluated Jsonnet output (without any transformations)
func Eval(dir string, opts Opts) (raw interface{}, err error) {
	r, _, err := eval(dir, opts.JsonnetOpts)
        if errors.Is(err, ErrNoEnv) {
                return r, err
        } else if err != nil {
                return nil, err
        }

	return r, nil
}

return r, nil
case ErrNoEnv:
log.Println(err)
return r, nil
default:
return nil, err
}
}
return r, nil
}