Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

Autofix with config #313

Merged
merged 5 commits into from
Oct 29, 2020
Merged

Autofix with config #313

merged 5 commits into from
Oct 29, 2020

Conversation

dani-santos-code
Copy link
Contributor

@dani-santos-code dani-santos-code commented Oct 19, 2020

Description

Closes #264

Type of change
  • Bug fix πŸ›
  • New feature ✨
  • This change requires a documentation update πŸ“–
  • Breaking changes ⚠️
How Has This Been Tested?
  • Manual tests
Checklist:
  • I have 🎩 my changes (A 🎩 specifically includes pulling down changes, setting them up, and manually testing the changed features and potential side effects to make sure nothing is broken)
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The test coverage did not decrease
  • I have signed the appropriate Contributor License Agreement


conf = setConfigFromFlags(cmd, conf)

allAuditors, err := all.Auditors(conf)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I just call it auditors? or configEnabledAuditors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Auditors makes sense to me! This code looks like it's based on the all command so I think we should change it there too!

Copy link
Contributor

@genevieveluyt genevieveluyt left a comment

Choose a reason for hiding this comment

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

Awesome this is very clean!


conf = setConfigFromFlags(cmd, conf)

allAuditors, err := all.Auditors(conf)
Copy link
Contributor

Choose a reason for hiding this comment

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

Auditors makes sense to me! This code looks like it's based on the all command so I think we should change it there too!

@@ -35,20 +47,42 @@ func autofix(cmd *cobra.Command, args []string) {
}
}

func loadKubeAuditConfigFromFile(configFile string) config.KubeauditConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it's based on loadConfigFromFile from the all command. I think that function was meant to use configFile (the passed in parameter) instead of auditAllConfig.configFile (since the passed in parameter is not used anywhere in the function 🀦 ). I do like loadKubeAuditConfigFromFile better as a function name though. So I think what we should do is keep this function but replace autofixConfig.kubeauditConfigFile with configFile, then remove the loadConfigFromFile function, and have the auditAll command call this function instead. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally!! it makes sense! I also failed to notice configFile wasn't being used! πŸ˜‚I will change it and also rename the function!

@@ -263,3 +264,13 @@ To write the fixed manifest to a different file, use the `--outfile/-o` flag:
```
kubeaudit autofix -f "manifest.yml" -o "fixed.yaml"
```

### Using Custom Rules with Kubeaudit Config File
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that two commands support the kubeaudit config maybe we can move the documentation from the all command (https://github.com/Shopify/kubeaudit/blob/master/docs/all.md#kubeaudit-config) to the home page (https://github.com/Shopify/kubeaudit#configuration-file) and then link there from the all and autofix readmes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea!!!

@ghost ghost self-requested a review October 26, 2020 15:47
README.md Outdated Show resolved Hide resolved
fix.go Outdated
@@ -1,135 +0,0 @@
package kubeaudit
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omg! I didn't mean to delete it! 😩I'll bring it back!

fixed.yaml Outdated
@@ -0,0 +1,37 @@
apiVersion: apps/v1beta2
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this file added intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noooo! sorry! I made a mistake when going back and forth! I should have reviewed this more carefully before asking for feedback!

Copy link
Contributor Author

@dani-santos-code dani-santos-code Oct 26, 2020

Choose a reason for hiding this comment

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

I was supposed to have deleted the yaml and ended up deleting the fix.go by accident instead!

Copy link
Contributor

@genevieveluyt genevieveluyt left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@dani-santos-code dani-santos-code merged commit abac30a into master Oct 29, 2020
@dani-santos-code dani-santos-code deleted the autofix-with-config branch October 29, 2020 15:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autofix should use a configuration file
2 participants