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

When creating civo firewall, --create-rules=false still creates default firewall rules #446

Open
chadmcrowell opened this issue Aug 6, 2024 · 5 comments

Comments

@chadmcrowell
Copy link

Issue

After creating the civo network "test-net", creating a civo firewall with the command civo fw create test-fw -n test-net --create-rules false, creates the default rules (same as when the --create-rules is true:

For example:

Screenshot 2024-08-06 at 08 12 57

Acceptance Criteria

  • When the user performs the command civo create fw with the flag --create-rules=false, there will be no rules created for the firewall.
@fernando-villalba
Copy link
Contributor

Yeah there is a couple of issues with this that we need to fix. In the meantime the work around is to run the command like this:

civo fw create test-fw -n test-net --create-rules=false

@fernando-villalba
Copy link
Contributor

So the "issue" here is the following:

--create-rules is a boolean flag. Generally speaking in CLI tools created in go, boolean flags require the = sign to change them. This is probably because they are intended to be used without any arguments, generally speaking.

So in our case, the issue is that our default is to create rules, so having a flag called --create-rules feels a little redundant. In reality we should have just created a flag called --no-default-rules or something like that instead. That way the user would have never need to run it as --no-default-rules=false and there would be no confusion.

These are some ways we can tackle this problem:

  • Create an additional --no-default-rules flag, leave the current one as legacy and perhaps remove it in the future.
  • Make it abundantly clear you need an equal sign for the current flag in the documentation. This could still lead to confusion.

I am partial to the first solution.

@Praveen005
Copy link
Contributor

Hi Team,

I worked on this issue, and would like to explain my approach for review.

In here,

  1. I created a new flag --no-default-rules as suggested above.
firewallCreateCmd.Flags().BoolVarP(&noDefaultRules, "no-default-rules", "", false, "the no-default-rules flag will ensure no default rules are created for the firewall, if is not defined will be set to false")
  1. Added a pre-run to set the value of createRules to false if flag --no-default-rules is specified.
        firewallCreateCmd.PreRunE = func(cmd *cobra.Command, args []string) error {
		if noDefaultRules {
			createRules =false
		}
		return nil
	}

I also updated the documentation:

Screenshot 2024-08-12 194812

Result:

  1. without --no-default-rules:
Screenshot 2024-08-12 193317
  1. with --no-default-rules:
Screenshot 2024-08-12 193423

Regards!

@Praveen005
Copy link
Contributor

Hi @uzaxirr,

Whenever you have a moment, could you please review this?

@uzaxirr
Copy link
Member

uzaxirr commented Sep 5, 2024

hii @Praveen005 Please create a PR

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

No branches or pull requests

4 participants