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

Add Terraform item #203

Merged
merged 5 commits into from
Sep 21, 2021
Merged

Add Terraform item #203

merged 5 commits into from
Sep 21, 2021

Conversation

radek-sprta
Copy link
Contributor

Description

Adds a new item - terraform - to show the terraform workspace. The colors are provisional, I can change them to something different. Terraform does not have an icon in nerd fonts, so I left it undefined.

Motivation and Context

For people who work a lot with Terraform, seeing the current workspace may prevent costly mistakes.

How Has This Been Tested

Tried with terraform on my installation. Correctly shows current workspace when inside terraform directory and nothing outside of it. I also added unit tests.

  • I have tested using Linux.
  • I have tested using MacOS.

Checklist

  • I have updated the documentation accordingly.
    I will update the wiki if changes are merged
  • I have updated the tests accordingly.

@IlanCosman
Copy link
Owner

A few things:

  1. Can you describe how to test this in reality? I.e what commands do I run to set up a simple terraform workspace?
  2. We probably don't want to display this when it just says default.
  3. Powerlevel10k uses $TF_WORKSPACE to do this task. Can we use that instead of (terraform workspace show)? It would be much faster because we wouldn't be interacting with terraform at all.

@radek-sprta
Copy link
Contributor Author

A few things:

1. Can you describe how to test this in reality? I.e what commands do I run to set up a simple terraform workspace?

You can quickly test it like this:

cd (mktemp -d)
terraform init
terraform workspace new test
terraform workspace select default
2. We probably don't want to display this when it just says `default`.

Good point.

3. Powerlevel10k uses `$TF_WORKSPACE` [to do this task](https://github.com/romkatv/powerlevel10k/blob/4f3d2ffe7251a17aa10e00ed30d55f6fd9d65002/internal/p10k.zsh#L4884-L4888). Can we use that instead of `(terraform workspace show)`? It would be much faster because we wouldn't be interacting with terraform at all.

$TF_WORKSPACE doesn't always exist - it's an override. Normally, the current workspace is in .terraform/environment (and the directory itself can be overriden with TF_DATADIR).
I have chosen to use terraform workspace show because it is future-proof, should anything change, and makes the code cleaner, while remaining fast. But if you want me to rewrite it using env variables and cat, let me know.

@IlanCosman
Copy link
Owner

IlanCosman commented Sep 21, 2021

$TF_WORKSPACE doesn't always exist...

Thanks for letting me know 😄 I agree with your decision to use terraform workspace show 👍

Alright, so the only things left to get this merged are:

  1. Make the item not display when it's default
  2. Add terraform to the tide_right_prompt_items of each config
  3. Add terraform to the items in _tide_remove_unusable_items.fish

Thanks for working on this!

P.S. I assume you chose 844FBA as the color because it was in the official branding. Great job 👍 I really appreciate that attention to detail. That's exactly what I wanted 😄

P.S.S:

But if you want me to rewrite it using env variables and cat, let me know.

You wouldn't use cat, you would use redirection like read terrform_workspace <$TF_DATADIR/environment.
But that's besides the point 😄

@radek-sprta
Copy link
Contributor Author

$TF_WORKSPACE doesn't always exist...

Thanks for letting me know smile I agree with your decision to use terraform workspace show +1

Alright, so the only things left to get this merged are:

1. Make the item not display when it's `default`

2. Add `terraform` to the `tide_right_prompt_items` of each config

3. Add `terraform` to the items in `_tide_remove_unusable_items.fish`

Thanks for working on this!

All done.

P.S. I assume you chose 844FBA as the color because it was in the official branding. Great job +1 I really appreciate that attention to detail. That's exactly what I wanted smile

Yes :)

P.S.S:

But if you want me to rewrite it using env variables and cat, let me know.

You wouldn't use cat, you would use redirection like read terrform_workspace <$TF_DATADIR/environment.
But that's besides the point smile

No love for cats 😿. I'll remember for future reference.

@IlanCosman IlanCosman merged commit 3787c72 into IlanCosman:main Sep 21, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants