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

fix: remove json indirection, restructure package #2

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

craigfurman
Copy link

@craigfurman craigfurman commented Dec 2, 2021

Why?

Extracting parsers that are used in several repos to this shared library makes a lot of sense! But, before we roll this out further (e.g. https://github.com/snyk/cloud-config-policy-engine/pull/379 and similar PRs), I want to take the opportunity to raise 2 issues, which I've attempted to fix in this PR:

First, the repository structure is unidiomatic. "pkg" is often found in the middle of a go subpackage hierarchy, but never at the end. This repo is already called "snyk-iac-parsers", so why not put parsers at the top level? Then, import declarations like parsers "github.com/snyk/snyk-iac-parsers/pkg" can simply become "github.com/snyk/snyk-iac-parsers", and the parsers package alias should be inferred automatically from this repo's package declarations.

Secondly, the JSON parser isn't really reusable: all it does is wrap errors with pkg/errors. Wrapping errors is application-specific, hence why we had the ParseJSON function in policy-engine in the first place, but putting it in this shared library isn't worth the mental overhead of indirection IMO. I'd advocate for updating all places that call parsers.ParseJSON to simply call json.Unmarshal from the stdlib, which has the same signature, and apply whatever error wrapping the application in question is doing.

What do you think of this? If this is agreed and merged, let's take the opportunity to update the consumers now too. It's better to do these annoying structural changes early while it's relatively cheap! I don't think we need to worry about proper semver for the release tag, this library is pre-1.0 and not used by anyone except us.

Changelog

fix: remove existing DS_Stores

They're already gitignored, but these must have made it into the repo
before that.

fix: remove JSON parser

The json parser just wraps the stdlib json parser with pkg/errors. This
isn't worth the indirection and dependency management: error wrapping
and structure is application-specific anyway, and a poor candidate for a
library.

fix: make directory structure more idiomatic

"pkg" is an idiomatic name for the middle part of a go package's path,
but is an unusual ending. This is already a parsers library and as far
as I know there is no expected need to house many disparate sibling
packages in here, so let's just use the top level.

Move test fixtures into the client package's testdata dir, as is also
idiomatic in go.

@craigfurman craigfurman requested a review from a team as a code owner December 2, 2021 17:47
@CLAassistant
Copy link

CLAassistant commented Dec 2, 2021

CLA assistant check
All committers have signed the CLA.

@craigfurman craigfurman marked this pull request as draft December 2, 2021 17:54
README.md Outdated Show resolved Hide resolved
Craig Furman added 3 commits December 3, 2021 09:37
They're already gitignored, but these must have made it into the repo
before that.
The json parser just wraps the stdlib json parser with pkg/errors. This
isn't worth the indirection and dependency management: error wrapping
and structure is application-specific anyway, and a poor candidate for a
library.
"pkg" is an idiomatic name for the middle part of a go package's path,
but is an unusual ending. This is already a parsers library and as far
as I know there is no expected need to house many disparate sibling
packages in here, so let's just use the top level.

Move test fixtures into the client package's `testdata` dir, as is also
idiomatic in go.
@craigfurman craigfurman marked this pull request as ready for review December 3, 2021 10:34
@craigfurman craigfurman changed the title fix: remove some redundant parser indirection fix: remove json indirection, restructure package Dec 3, 2021
@craigfurman
Copy link
Author

@teodora-sandu and I chatted about this on slack. @muratcelep, would you mind reviewing this? See justification in the "why?" section.

@craigfurman
Copy link
Author

I'm only hesitating on the CLA because I'm not sure it makes sense, given I'm an org member! Should I sign it?

Copy link
Contributor

@muratcelep muratcelep left a comment

Choose a reason for hiding this comment

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

@craigfurman lgtm!

@teodora-sandu
Copy link
Contributor

I think every time we add a new CLA we need to sign it - I've definitely signed it every time I created a new public repo @craigfurman

@teodora-sandu teodora-sandu removed their request for review December 3, 2021 14:07
@teodora-sandu teodora-sandu merged commit bf9a4bd into main Dec 7, 2021
@teodora-sandu teodora-sandu deleted the fix/rm-redundant-indirection branch December 7, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants