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 remote ruleset to woke configuration #28

Merged
merged 12 commits into from
Aug 5, 2021
Merged

Conversation

mkcomer
Copy link

@mkcomer mkcomer commented Aug 3, 2021

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

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

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

What is the new behavior (if this is a feature change)?
This PR adds functionality to pull in remote rules from urls - https.

To test this PR:
go build
go run main.go -c "https://raw.githubusercontent.com/get-woke/woke/main/example.yaml" --debug

To test a 404 URL:
go run main.go -c "https://raw.githubusercontent.com/get-woke/woke/main/example" --debug

@mkcomer mkcomer marked this pull request as draft August 3, 2021 14:42
@mkcomer mkcomer linked an issue Aug 3, 2021 that may be closed by this pull request
4 tasks
@mkcomer mkcomer marked this pull request as ready for review August 3, 2021 16:51
Copy link

@jeremydelacruz jeremydelacruz left a comment

Choose a reason for hiding this comment

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

Just a couple of questions and suggestions, but looks good to me!

_, err = io.Copy(out, resp.Body)
return err
} else {
return fmt.Errorf("unable to download remote config from url - http response code: %v", resp.StatusCode)

Choose a reason for hiding this comment

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

Totally agree with returning the error here -- maybe we should also log.Error() the msg/body of the http response here as well? Just thinking it might provide more detailed information in case the remote server sent relevant info in the response body

Copy link
Author

Choose a reason for hiding this comment

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

Added in the response body to the error message!

})

t.Run("invalid-url", func(t *testing.T) {
boolResponse := isValidURL("/Users/Document/test.yaml")

Choose a reason for hiding this comment

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

Out of curiosity, do you know if something like C:\user\test.yaml or C:\directory.com\test.yaml returns valid? Unsure how the URL validation works under the hood and just thinking of some weird edge cases

Copy link
Author

Choose a reason for hiding this comment

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

it returns invalid - adding in some more unit tests


// downloads file from url to set filepath
func DownloadFile(filepath string, url string) error {
// Removing gosec lint warning - as we have to pass in user specified url for remote config

Choose a reason for hiding this comment

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

This comment looks like it's from when we were doing http.Get() to document that we were suppressing the linting warnings -- is it still needed?

Copy link
Author

Choose a reason for hiding this comment

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

Good call - forgot to remove the warning !

log.Debug().Str("filename", filename).Msg("Saved remote config to local file.")
}

// TO DO = Need to add more error handling on reading/validating yamlFile file - issue # 16

Choose a reason for hiding this comment

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

I think this is fine keeping the TODO item here for visibility, but keep in mind referencing issue #16 upstream won't make sense.

Copy link
Author

Choose a reason for hiding this comment

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

good call - i may just remove for now

Copy link

@kevinleung23 kevinleung23 left a comment

Choose a reason for hiding this comment

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

PR looks good! I suggest adding a section into the README docs to explain the functionality and how to use the CLI command.

Other than documentation code is readable.

@mkcomer mkcomer merged commit 0e1be43 into main Aug 5, 2021
mkcomer added a commit that referenced this pull request Aug 5, 2021
mkcomer added a commit that referenced this pull request Aug 5, 2021
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.

Ability to pull in rules from remote repos
3 participants