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

feat: add support for minikube, terragrunt, tgenv #1014

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

yinchuandong
Copy link

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
New Features:

  1. Add minikube dashboard subcommand
  2. Add terragrunt support
  3. Add tgenv support

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?
image

image

image

Additional info:

@withfig-bot
Copy link
Collaborator

withfig-bot commented Mar 8, 2022

Overview

src/terragrunt.ts:

Info:

src/tgenv.ts:

Info:

Script:
tgenv list
postProcess(function):

 function (out) {
      return out
        .trim()
        .split("\n")
        .map((tfversion) => {
          return { name: tfversion, description: "Version" };
        });
    }

Script:
tgenv list-remote
postProcess(function):

 function (out) {
      return out
        .trim()
        .split("\n")
        .map(function (line) {
          return { name: line, type: "option" };
        });
    }

src/minikube.ts:

Info:

@withfig-bot
Copy link
Collaborator

Hello @yinchuandong,
thank you very much for creating a Pull Request!
Here is a small checklist to get this PR merged as quickly as possible:

  • Do all subcommands / options which take arguments include the args property (args: {})?
  • Are all options modular? E.g. -a -u -x instead of -aux
  • Have all other checks passed?

Please add a 👍 as a reaction to this comment to show that you read this.

@yinchuandong yinchuandong changed the title Add support for minikube, terragrunt, tgenv feat: add support for minikube, terragrunt, tgenv Mar 8, 2022
fedeci
fedeci previously requested changes Mar 8, 2022
Copy link
Contributor

@fedeci fedeci left a comment

Choose a reason for hiding this comment

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

Are options with the equal valid when no equal is specified? If they are not we should add requiresEqual: true

src/terragrunt.ts Show resolved Hide resolved
src/tgenv.ts Outdated Show resolved Hide resolved
src/tgenv.ts Outdated Show resolved Hide resolved
src/minikube.ts Show resolved Hide resolved
src/terragrunt.ts Outdated Show resolved Hide resolved
src/terragrunt.ts Outdated Show resolved Hide resolved
src/terragrunt.ts Outdated Show resolved Hide resolved
src/terragrunt.ts Outdated Show resolved Hide resolved
src/terragrunt.ts Outdated Show resolved Hide resolved
src/terragrunt.ts Outdated Show resolved Hide resolved
@fedeci
Copy link
Contributor

fedeci commented Mar 8, 2022

Thanks for opening this PR!

Comment on lines 244 to 246
args: {
template: "filepaths",
suggestCurrentToken: true,
Copy link
Author

Choose a reason for hiding this comment

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

Nice feature! Thanks for the suggestion!
image

@yinchuandong
Copy link
Author

Are options with the equal valid when no equal is specified? If they are not we should add requiresEqual: true

Hi @fedeci , these terragrunt global options don't require equal. referring to https://terragrunt.gruntwork.io/docs/reference/cli-options/#all-terraform-built-in-commands

Copy link
Contributor

@fedeci fedeci left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @yinchuandong! If the equals are not mandatory can you remove the insertValues that insert those? I honestly find cleaner options without equals.

@yinchuandong
Copy link
Author

Thanks for the changes @yinchuandong! If the equals are not mandatory can you remove the insertValues that insert those? I honestly find cleaner options without equals.

Hi @fedeci , thanks for your suggestion! I got what you mean. But terragrunt is a little bit tricky. It's a wrapper based on terraform which requires equal whileterragrunt itself doesn't require equal. Terragrunt needs to be compatible with Terraform and that's the reason why you see some options have equal while some don't have. I cleaned some commands based on your suggestions via directly importing from existing the terraform spec. Could you pls have a look again? It would be great if I can get this pr merged asap. We can make some improvements in the next branch. I have some DevOps friends who are keen to use the Terragrunt feature. And to be honest, most global options are not frequently used on a daily basis.

image

Reference

  1. https://terragrunt.gruntwork.io/docs/reference/cli-options/#all-terraform-built-in-commands

@mschrage mschrage dismissed fedeci’s stale review March 15, 2022 23:23

We'll move insertionValue changes to another PR :)

@mschrage
Copy link
Member

Hey @yinchuandong! Sorry for the slow response here. I am not sure why this wasn't merged in. Could you sign our CLA (and ideally fix the conflicts) and I will merge this ASAP.

@mschrage
Copy link
Member

recheck

@withfig-bot
Copy link
Collaborator

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

@brendanfalk
Copy link
Member

Hi @yinchuandong - know it's been a while, but we would love to get this merged! Could you please sign the CLA and get these small conflicts fixed? We'd love to give you merge credit :)

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.

None yet

5 participants