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

Jump to bottom

feat(cli): Add --with-prune option for diff command #469

Merged
merged 11 commits into from
Jan 17, 2021
Merged

feat(cli): Add --with-prune option for diff command #469

merged 11 commits into from
Jan 17, 2021

Conversation

curusarn
Copy link
Contributor

@curusarn curusarn commented Jan 7, 2021 •
edited by sh0rez
Loading

Fixes #416 in line with what @malcolmholmes suggested.

Adds --with-prune option to diff command. This makes it possible to see a full diff of both added and deleted resources.

I'm not sure if it's okay to add --ignore-not-found to GetByState() or if it should be passed as argument to it. I have left a TODO in the code.

Let me know if I should change anything.

@curusarn curusarn changed the title Feature: Add --with-prune option for diff command feat(cli): Add --with-prune option for diff command Jan 7, 2021
sh0rez
sh0rez requested changes Jan 7, 2021
View reviewed changes
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.

Wow, thanks for jumping onto this! I just tested your code and it works really well!

I noticed an edge-case that occurs when there are no resources that carry the tanka.dev/environment label, kubectl returns simply nothing on stdout because of the --ignore-not-found flag. The following logic does not handle that case, JSON parsing fails.

@curusarn
Copy link
Contributor Author

curusarn commented Jan 8, 2021 •
edited
Loading

Thanks for your quick reaction!

I have implemented the requested changes.

One side-effect of this PR is that tk prune no longer fails if there are new k8s objects in tanka code that were not applied yet. I think this is an improvement but it is a change in behavior so it shouldn't go unacknowledged/undiscussed. It didn't cause any issues during my testing but I felt like I should bring this up anyway.

sh0rez
sh0rez approved these changes Jan 10, 2021
View reviewed changes
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.

Super nice work! Congrats on your first PR 🎉

@sh0rez sh0rez force-pushed the feature_diff_opt_with_prune branch from d3c8b4b to 00ac1a2 Compare January 17, 2021 17:25
@sh0rez sh0rez merged commit 5d821cc into grafana:master Jan 17, 2021
@craigfurman
Copy link
Contributor

craigfurman commented Feb 1, 2021 •
edited
Loading

I was about to open an issue to discuss adding "ignore not found" behaviour to prune, so that we can see prune diffs when unapplied changes exist in the rendered manifests, and found this. Putting that behind the same command as diff is even better!

I look forward to being able to use this - any chance of a release soon? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sh0rez sh0rez sh0rez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Is there a way to get a "full diff" before running BOTH tk apply and tk prune?
3 participants
@curusarn @craigfurman @sh0rez