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

fix: mimic --tla-code behavior from jsonnet for functions in main.jsonnet in env discovery #1251

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

sabeechen
Copy link
Contributor

@sabeechen sabeechen commented Nov 29, 2024

Problem

Some parts of tanka behave inconsistently with standard jsonnet when using inline environments with top level arguments. The differences can be illustrated by this inline environment:

main.jsonnet

function(foo="bar", fizz='buzz') {
  apiVersion: 'tanka.dev/v1alpha1',
  kind: 'Environment',
  // Other env stuff
}

And then running:

$ tk env list main.jsonnet
NAME    NAMESPACE    SERVER    

which finds no environments. Similarly running:

$ tk export export-dir main.jsonnet --recursive
{"level":"info","parallelism":8,"envs":0,"time":"2024-11-29T13:28:23-07:00","message":"Reducing parallelism to match number of environments"}

will export no manifests. For both commands, appending --tla-code foo=1 will resolve the issue, listing the environment and exporting manifests.

If instead main.jsonnet is a function taking no arguments, eg:

main.jsonnet

function() {
  apiVersion: 'tanka.dev/v1alpha1',
  kind: 'Environment',
  // Other env stuff
}

Then there is no combination of arguments that will cause tk env list or tk export --recursive to succeed. It will either do nothing or fail when a --tla-code argument is provided.

Expectation

These tk env list and tk export should permit the same flexibility as underlying jsonnet, namely that:

  • A main.jsonnet whose TLA's are all optional should be usable without specifying any --tla-code arguments.
  • A main.jsonnet with a function that takes no arguments should be useable.
  • Calling tk env list main.jsonnet --tla-code badargument=0 on a mainfaile that isn't a function ignores the tla-code argument.
    This is the behavior I'd expect to see from tk, because its also how calling jsonnet works.

My current workaround for this is to always add a dummy unused TLA to my main.jsonnet and ensure that all my tk commands are run with --tla-code unused=0, which is cumbersome.

Cause

The root cause of this appears to be in how the EvalScript for finding environments is constructed. The presence of any --tla-code argument is used to determine if main.jsonnet should be used called as a function or not. This decision logic behaves incorrectly if a --tla-code is provided with a non-function main.jsonnet or if no --tla-code argument is provided to a main.jsonnet that has no required arguments.

Solution in this PR

Before running an EvalScript, jsonnet.Evaulate is called with std.isFunction on the imported mainfile to determine if it needs to be called as a function. This adds an extra evaluation every time an EvalScript is used, but since thats typically only done once per tk call and the std.isFunction doesn't manifest anything except a boolean, I'd expect it to have a minimal impact on performance.

@sabeechen sabeechen requested a review from a team as a code owner November 29, 2024 21:09
@sabeechen
Copy link
Contributor Author

Looking through issues in this repo, I believe this will resolve:

And although it perpetuates the "hack" for finding environments without manifesting their data, I think it also solves it without having to do any complex AST parsing by just leaning into jsonnet VM to do the real work.

@guicaulada guicaulada requested review from zerok and Duologic and removed request for a team February 4, 2025 19:19
Copy link
Contributor

@zerok zerok left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thank you very much and sorry for the late review!

@zerok zerok changed the title Mimic --tla-code behavior from jsonnet for functions in main.jsonnet during envrionment discovery fix: mimic --tla-code behavior from jsonnet for functions in main.jsonnet during environment discovery Feb 6, 2025
@zerok zerok changed the title fix: mimic --tla-code behavior from jsonnet for functions in main.jsonnet during environment discovery fix: mimic --tla-code behavior from jsonnet for functions in main.jsonnet during env discovery Feb 6, 2025
@zerok zerok enabled auto-merge February 6, 2025 08:53
@zerok zerok added this pull request to the merge queue Feb 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 6, 2025
@zerok zerok added this pull request to the merge queue Feb 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 6, 2025
@zerok zerok changed the title fix: mimic --tla-code behavior from jsonnet for functions in main.jsonnet during env discovery fix: mimic --tla-code behavior from jsonnet for functions in main.jsonnet in env discovery Feb 6, 2025
@zerok zerok enabled auto-merge February 6, 2025 09:10
@zerok zerok added this pull request to the merge queue Feb 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 6, 2025
@zerok zerok added this pull request to the merge queue Feb 6, 2025
Merged via the queue into grafana:main with commit 3065778 Feb 6, 2025
27 of 28 checks passed
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