Skip to content
This repository has been archived by the owner on Dec 26, 2023. It is now read-only.

[WIP-FIX] Add a --auto-approve flag to layerform kill #73

Closed
wants to merge 3 commits into from
Closed

[WIP-FIX] Add a --auto-approve flag to layerform kill #73

wants to merge 3 commits into from

Conversation

hi-hi-ray
Copy link

@hi-hi-ray hi-hi-ray commented Sep 10, 2023

Why this matters

Add a --auto-approve flag to the kill command that makes the cli skip the confirmation when killing a layer instance.

Solution

  • Fixed typo error
  • Add kill flag
Notes

The Flag check was already created in the code.
Related to: #62

Copy link
Member

@vieiralucas vieiralucas left a comment

Choose a reason for hiding this comment

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

Hey, hello @hi-hi-ray.

Thank you so much for the PR!

I think you forgot to change it so that the flag get's passed to the Kill::Run method.

Here:
https://github.com/ergomake/layerform/blob/bf01ccddef104e7631b647fb1d0f1645f5997f97/cmd/cli/kill.go#L60
It is currently hardcoded to pass false, this PR should change this to get the value from the flag.
Something like this:

		autoApprove, err := cmd.Flags().GetBool("auto-approve")
		if err != nil {
			fmt.Fprintf(os.Stderr, "%s\n", errors.Wrap(err, "fail to get --auto-approve flag, this is a bug in layerform"))
			os.Exit(1)
			return
		}

		err = kill.Run(ctx, layerName, instanceName, autoApprove, vars)

Lastly, I think it would be pretty cool if we change the e2e.yml tests that test the kill command to use that flag instead of using the yes command.

Over here:
https://github.com/ergomake/layerform/blob/bf01ccddef104e7631b647fb1d0f1645f5997f97/.github/workflows/e2e.yml#L80-L85
Just change line 82 to use the new flag

          layerform kill bar test_bar --auto-approve

PS: thank you for fixing that typo ❤️

@hi-hi-ray
Copy link
Author

hi-hi-ray commented Sep 11, 2023

I will work this week with your suggestions. Thank You so much for the help!!

@lucasfcosta
Copy link
Member

lucasfcosta commented Sep 17, 2023

Hey @hi-hi-ray let us know if you need any help on this, happy to provide any further guidance if necessary! And thanks again for contributing. Please don't feel the need to rush this, just thought I'd check in to make sure you get help if you need anything 🙌

@hi-hi-ray
Copy link
Author

I'll try to work on the suggestions until next week, sorry for disappearing.

@hi-hi-ray
Copy link
Author

hi-hi-ray commented Oct 1, 2023

Hey, would you mind to put a label of WIP on this PR?

And i'm working on the requests =]

@hi-hi-ray hi-hi-ray changed the title Add a --auto-approve flag to layerform kill [WIP-FIX] Add a --auto-approve flag to layerform kill Oct 1, 2023
@hi-hi-ray hi-hi-ray closed this by deleting the head repository Jan 7, 2024
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.

3 participants