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

Don't crash if an environment directory is missing spec.json #108

Merged
merged 2 commits into from
Nov 17, 2019

Conversation

abh
Copy link
Contributor

@abh abh commented Nov 12, 2019

No description provided.

@sh0rez
Copy link
Member

sh0rez commented Nov 12, 2019

Hey, thanks for your contribution! However, the issue you are trying to fix actually lies in the findBaseDirs function, which should never return invalid environments. I suggest the following:

diff --git a/cmd/tk/other.go b/cmd/tk/other.go
index f160d8c..fc86aa5 100644
--- a/cmd/tk/other.go
+++ b/cmd/tk/other.go
@@ -20,7 +20,10 @@ func findBaseDirs() (dirs []string) {
 	}
 
 	if err := filepath.Walk(".", func(path string, info os.FileInfo, err error) error {
-		if _, err := os.Stat(filepath.Join(path, "main.jsonnet")); err == nil {
+		if _, err := os.Stat(filepath.Join(path, "main.jsonnet")); err != nil {
+			return nil
+		}
+		if _, err := os.Stat(filepath.Join(path, "spec.json")); err == nil {
 			dirs = append(dirs, path)
 		}
 		return nil

Would you adapt your PR? Thanks!

@sh0rez sh0rez added component/cli Command Line Interface kind/bug Something isn't working labels Nov 12, 2019
@sh0rez
Copy link
Member

sh0rez commented Nov 16, 2019

Hey, are you still on this? Let me know if I should pick up :D

@abh
Copy link
Contributor Author

abh commented Nov 17, 2019

Hi @sh0rez -- I came across this when evaluating how much work it'd be to get my old ksonnet setup working with tanka; after fixing this and getting a little further I realized it was more work than I had time for (and I haven't gotten back to it).

Anyway, I added a new commit based on your feedback. I fixed it a little differently than what you suggested but I think this way shows the intent more clearly.

Copy link
Member

@sh0rez sh0rez left a comment

Choose a reason for hiding this comment

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

LGTM!

Just out of interest, what would have had to be done to migrate to Tanka? We designed tk to be very similar to ks so that it should be a flawless experience for most users .. could you elaborate a bit so we could improve? :D

@sh0rez sh0rez merged commit 9bd15e6 into grafana:master Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/cli Command Line Interface kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants