feat: inline Environment #403

Duologic · 2020-10-11T15:47:01Z

Building further on #389, with implementing the environment inline there is no more need to encapsulate a Tanka Environment in a directory.

This proposes a solution for #390.

Might also help for #394 as the "spec" is now evaluated in Jsonnet.

This PR will allow us to use the Environment inline, like this:

{
  apiVersion: 'tanka.dev/v1alpha1',
  kind: 'Environment',
  metadata: {
    name: 'myenv',
  },
  spec: {
    apiServer: 'https://localhost',
    namespace: 'myns',
  },
  data: {
    // put here what used to be in main.jsonnet
    // or just (import 'main.jsonnet') and call this file with tanka directly
  },
}

It is fully backwards compatible but has a few drawbacks:

The tanka.dev/Environment extCode is based on the premise that the env is availabe before runtime, it is now generated at runtime, it doesn't make much sense to read env from tk.env now as the env is now available inside your jsonnet anyway.
~~tk env is build upon the spec.json, it will be hard to support inline environments with this~~ see below
tk complete might need some adjustments

On tk env:

tk env add and tk env remove will be fairly easy to support, just need to decide on what the default layout is for add.
tk env list ~~can become significantly slower as the jsonnet has to be evaluated for an Environment object to exist, we can improve on speed with go routines and only evaluate if spec.json doesn't exists~~ I've fixed this by wrapping a snippet around the jsonnet removing the data: object in 2ee1696
tk env set would be really hard to port over, you'd be modifying someone's jsonnet code.

Duologic · 2020-10-19T08:52:13Z

rebased.

pkg/spec/v1alpha1/config.go

Duologic · 2020-10-25T09:37:59Z

rebased.

jdbaldry · 2020-10-26T09:30:43Z

Also, I think the current main.jsonnet/spec.json approach is actually very beginner friendly
I agree, I don't think we should change the defaults.

My first thought is that option 1. is closer to existing behavior which I like but I'm also keen on how 2. looks aesthetically. 2. adjusts the relationship between the environment and data (environment contains data rather than being composed with it), and I'm not sure if there is enough value to justify confusing that relationship.

That being said I'm not sure which of the two relationships is objectively better out of:

environment contains data
environment and data are composed together

malcolmholmes · 2020-10-26T13:20:37Z

That being said I'm not sure which of the two relationships is objectively better out of:

environment contains data

environment and data are composed together

To really get a sense of where this is going, it helps to look at the next step that @Duologic has in mind, where you can have multiple environments within one setup. That I think will drive exactly how this is expressed.

Duologic · 2020-10-26T20:20:56Z

I'm wondering whether we should do it in one go but I'm worried this PR becomes to big.

Duologic · 2020-10-29T16:55:53Z

Please have a look at #414, which can replace this PR.

captncraig · 2020-10-29T19:47:12Z

I really like the possibilities that this brings. A few thoughts:

I feel it is fair to assume there will be either a spec.json or a main.jsonnet that evaluates to a single environment having spec and data fields. I like enforced structure and prefer checked assumptions to supporting any desired style.
If we don't detect a spec.json, we can assume we are in the "new" mode and do a first pass that evaluates .spec to get all of the metadata we need. That will let us support tk env and friends with less overhead, as well as keeping the extCodes working and things.
I am still not sold on feat: multiple inline environments #414, since running multiple environments is a much larger paradigm shift in the workflows around tanka. It is a lot more involved than slightly refactoring the input format.

sh0rez · 2020-11-02T11:52:40Z

I'm a bit out here and surely didn't attend all discussions, so I might miss obvious things.

I'm generally on the lines of @captncraig, where I am in favor of a slight refactor / workflow change (this PR), but #414 seems more far-reaching to me. I do not fully understand the use-case for #414, but I am sure @Duologic can explain that to me.

Some random thoughts I had while reading along:

If we don't detect a spec.json, we can assume we are in the "new" mode

Existing behavior explicitly supports not having a spec.json, resulting in namespace=default and an empty apiServer url. We need to take care not to break users in unexpected ways.

either a spec.json or a main.jsonnet

We introduce more than one way to do the same thing here, mostly for compatibility reasons.

I'd really like to avoid a "basic" and "advanced" mode. Could we instead identify what makes the Jsonnet mode harder and make it much simpler so we can ship it as the one and only way?

Duologic · 2020-11-02T13:22:21Z

I'm generally on the lines of @captncraig, where I am in favor of a slight refactor / workflow change (this PR), but #414 seems more far-reaching to me. I do not fully understand the use-case for #414, but I am sure @Duologic can explain that to me.

As mentioned in #414, it proposes an answer to the question "What if the jsonnet has multiple environments?" This can also be answered with an error message saying Tanka won't handle multiple environments at once, that seems a bit dense to me as it is perfectly possible to handle multiple environments at once.

I'd really like to avoid a "basic" and "advanced" mode. Could we instead identify what makes the Jsonnet mode harder and make it much simpler so we can ship it as the one and only way?

The "advanced mode" does not necessarily reflect on Tanka, the way of working (basic/advanced) becomes a matter of applying conventions to your jsonnet code base instead of imposing restrictions by the runtime (Tanka). With saying basic, I mean defining the current approach as the simplest convention possible on how to use Tanka. If we want to bring out a different convention as the basic use case, then we can, for both the single environment and multiple environments pull requests.

There are some implementation details I've put into place on #414 (like finding the environments), I'd like to backport to this PR in case #414 doesn't get accepted. This would allow us to simply pull all Environments out of the jsonnet code, and if there is more than 1, we can return aforementioned error instead of trying to apply them. done

Duologic · 2020-11-04T13:40:51Z

re data::

As I said before, the argument for making data part of the Environment object is to ensure the jsonnet is
deterministic about what it represents.

Any other the Jsonnet objects can exist as siblings to the Environment object, removing the assumption that all visible
objects are Kubernetes objects. This brings opportunities here for other tools to jump less hoops for the same jsonnet
codebase. (grizzly hacks in making $.grafanaDashboards visible at runtime, for example)

re spec.json behavior:

The code currently looks for an Environment object, if that doesn't exist it assumes the old spec.json behavior. This is
required to keep backwards compatibility. We can deprecate that way of working over time, but that doesn't need to be
decided here.

This moves the naming to the ParseDir() function as naming based on directory name only makes sense in the context of spec.json. Inline environments can technically be run form anywhere, only convention keeps them in this assumed directory structure.

pkg/tanka/parse.go

pkg/spec/v1alpha1/config.go

sh0rez · 2020-11-18T12:07:55Z

pkg/tanka/parse.go

+type evaluateFunc func(path string, opts jsonnet.Opts) (string, error)
+
+// parseEnv finds the Environment object at the given path
+func parseEnv(path string, opts jsonnet.Opts, evalFn evaluateFunc) (interface{}, *v1alpha1.Config, error) {


Injecting big amounts of logic as a anonymous function is very hard to debug (behavior no longer obvious from function body). I will work with @Duologic to find a better way here. I wish to hold this PR until that happened, if that's okay (@malcolmholmes)

I'm happy with the functionality of this PR as it is. I agree with the code being a bit more complex than it needs to be, but am also happy to see that addressed in a subsequent PR.

sh0rez

Looks good! Some minor findings and this is good to be merged

pkg/spec/v1alpha1/environment.go

sh0rez · 2020-11-19T15:10:14Z

pkg/tanka/workflow.go

 func Eval(dir string, opts Opts) (raw interface{}, err error) {
 	r, _, err := eval(dir, opts.JsonnetOpts)
-	if err != nil {
-		return nil, err
-	}
-	return r, nil
+	return r, err
 }


What's the reason for removing this? Well behaved functions should not return data in case of errors

If r is not nil, then the evaluation succeeded, the error could still be that there is no Environment but still evaluate the jsonnet fine. Do you think I should catch that err type and only return r, nil if it isn't about the missing environment?

PTAL, I catch a few errors that have no effect on the eval itself and print them as log messages instead.

pkg/tanka/parse.go

sh0rez · 2020-11-19T15:13:31Z

pkg/tanka/errors.go

+}
+
+// ErrMultipleEnvs means that the given jsonnet has multiple Environment objects
+type ErrMultipleEnvs struct {


Could this error message include the names of the environments? Makes it far easier to debug then

sh0rez · 2020-11-19T15:14:11Z

pkg/spec/spec.go

 	config := v1alpha1.New()
 	if err := json.Unmarshal(data, config); err != nil {
 		return nil, errors.Wrap(err, "parsing spec.json")
 	}

-	// set the name field
-	config.Metadata.Name = name


If we no longer set the name ourselves, we should enforce some name is set

I've covered for this in 4ee8de7

sh0rez · 2020-11-20T09:52:28Z

pkg/tanka/workflow.go

+	if err != nil {
+		switch err.(type) {
+		case ErrMultipleEnvs:
+			log.Println(err)


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

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

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

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

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 }

Duologic changed the title ~~feat: allow inline Environment~~ feat: inline Environment Oct 11, 2020

Duologic marked this pull request as ready for review October 11, 2020 16:18

Duologic requested review from sh0rez and malcolmholmes October 11, 2020 16:18

Duologic mentioned this pull request Oct 12, 2020

feat: allow other entrypoints #389

Merged

Duologic force-pushed the duologic/inline_environment branch from 06a5a04 to 0124866 Compare October 19, 2020 08:51

sh0rez reviewed Oct 24, 2020

View reviewed changes

pkg/spec/v1alpha1/config.go Outdated Show resolved Hide resolved

allow inline Environment

993c559

Duologic force-pushed the duologic/inline_environment branch from 0124866 to 993c559 Compare October 25, 2020 08:53

Duologic mentioned this pull request Oct 29, 2020

feat: multiple inline environments #414

Closed

Duologic added 4 commits November 4, 2020 14:41

refactor: evalJsonnet -> eval

5c506a3

refactor: use spec.Parse

b18a386

This moves the naming to the ParseDir() function as naming based on directory name only makes sense in the context of spec.json. Inline environments can technically be run form anywhere, only convention keeps them in this assumed directory structure.

refactor: extract Environment as k8s-like object

ac417d3

fix(cli): tk env list for inline envs

2ee1696

Duologic force-pushed the duologic/inline_environment branch from 060e0cc to 2ee1696 Compare November 6, 2020 00:30

Duologic requested a review from sh0rez November 6, 2020 15:22

fix: allow eval with err

12ece72

malcolmholmes approved these changes Nov 18, 2020

View reviewed changes

pkg/tanka/parse.go Outdated Show resolved Hide resolved

pkg/tanka/parse.go Outdated Show resolved Hide resolved

pkg/spec/v1alpha1/config.go Outdated Show resolved Hide resolved

sh0rez requested changes Nov 18, 2020

View reviewed changes

s/v1alpha1.Config/v1alpha1.Environment

943565f

Duologic added 2 commits November 18, 2020 17:46

s/extract/extractedEnvs

695e7a3

docs: update function descriptions

b6ae81b

sh0rez approved these changes Nov 19, 2020

View reviewed changes

Duologic added 5 commits November 20, 2020 09:58

fix: return env names with error

065a3e1

fix: log expected errors on eval

f16c942

fix: ensure metadata.name is set on loading an env

4ee8de7

doc: remove out of place docstring

6533972

fix: omitempty Data

4f11ab2

Duologic requested a review from sh0rez November 20, 2020 09:27

sh0rez reviewed Nov 20, 2020

View reviewed changes

Duologic added 2 commits November 26, 2020 21:16

conditional return values

9c30b46

no switch inside if

52ba439

Duologic merged commit 12f7716 into master Nov 26, 2020

Duologic deleted the duologic/inline_environment branch November 26, 2020 21:07

sh0rez mentioned this pull request Dec 30, 2020

Resolve spec.apiServer from kubectl context at runtime #394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: inline Environment #403

feat: inline Environment #403

Duologic commented Oct 11, 2020 •

edited

Loading

Duologic commented Oct 19, 2020

Duologic commented Oct 25, 2020

jdbaldry commented Oct 26, 2020

malcolmholmes commented Oct 26, 2020 •

edited by Duologic

Loading

Duologic commented Oct 26, 2020

Duologic commented Oct 29, 2020

captncraig commented Oct 29, 2020

sh0rez commented Nov 2, 2020

Duologic commented Nov 2, 2020 •

edited

Loading

Duologic commented Nov 4, 2020

sh0rez Nov 18, 2020

malcolmholmes Nov 19, 2020

sh0rez left a comment

sh0rez Nov 19, 2020

Duologic Nov 19, 2020

Duologic Nov 20, 2020

sh0rez Nov 19, 2020

Duologic Nov 20, 2020

sh0rez Nov 19, 2020

Duologic Nov 20, 2020

sh0rez Nov 20, 2020

Duologic Nov 21, 2020

sh0rez Nov 21, 2020

Duologic Nov 21, 2020

sh0rez Nov 21, 2020

feat: inline Environment #403

feat: inline Environment #403

Conversation

Duologic commented Oct 11, 2020 • edited Loading

Duologic commented Oct 19, 2020

Duologic commented Oct 25, 2020

jdbaldry commented Oct 26, 2020

malcolmholmes commented Oct 26, 2020 • edited by Duologic Loading

Duologic commented Oct 26, 2020

Duologic commented Oct 29, 2020

captncraig commented Oct 29, 2020

sh0rez commented Nov 2, 2020

Duologic commented Nov 2, 2020 • edited Loading

Duologic commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sh0rez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duologic commented Oct 11, 2020 •

edited

Loading

malcolmholmes commented Oct 26, 2020 •

edited by Duologic

Loading

Duologic commented Nov 2, 2020 •

edited

Loading