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 metadata to manifest & warn on unknown fields #528

Merged
merged 4 commits into from May 9, 2017

Conversation

Projects
None yet
4 participants
@darkowlzz
Copy link
Collaborator

darkowlzz commented May 8, 2017

  • Use manifest buffer to create a map of TomlTree and find all the
    unknown fields in the tree. Raise warnings for unknown fields.
  • For "metadata" field, ensure it's of map type, else raise warning.

Fixes #506

@googlebot googlebot added the cla: yes label May 8, 2017

manifest.go Outdated
}
}
if !isValidMetadata {
internal.Logf("WARNING: metadata should consist of key-value pairs")

This comment has been minimized.

@sdboyer

sdboyer May 8, 2017

Member

We're probably going to end up wanting to reuse this func elsewhere, in places where it's not a good idea to log directly. So, instead of directly logging the issues, could we switch to a ([]error, error) return type, and record all the problems we detect as items in the first slice?

Add metadata to manifest & warn on unknown fields
- Use manifest buffer to create a map of TomlTree and find all the
unknown fields in the tree. Raise warnings for unknown fields.
- For "metadata" field, ensure it's of map type, else raise warning.

@darkowlzz darkowlzz force-pushed the darkowlzz:506-add-metadata-warn-unknown-manifest-fields branch 2 times, most recently from 64abf1d to 90656bc May 8, 2017

Return []error from ValidateManifest and add tests
- Modifies ValidateManifest() to return []error, as a collection of all
the errors, instead of logging directly.
- Adds tests for ValidateManifest().

@darkowlzz darkowlzz force-pushed the darkowlzz:506-add-metadata-warn-unknown-manifest-fields branch from 90656bc to 8129217 May 8, 2017

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 8, 2017

  • Changed validateManifest() return type to ([]error, error).
  • Added tests for validateManifest().
@sdboyer
Copy link
Member

sdboyer left a comment

just a bit more...

{
tomlString: `
[[dependencies]]
name = "github.com/foo/bar"

This comment has been minimized.

@sdboyer

sdboyer May 9, 2017

Member

can we also validate that there aren't any extra fields within [[dependencies]] or [[overrides]] items?

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 9, 2017

@sdboyer thanks to that extra bit, I found that the checks can be improved by just using type assertion. Once a property is TOML array of tables ([[some-name]]), TOML ensures that the content has to be map (key-value pair) :)

Added checks for invalid fields in [[dependencies]] or [[overrides]].

@darkowlzz darkowlzz force-pushed the darkowlzz:506-add-metadata-warn-unknown-manifest-fields branch 4 times, most recently from 1bd28e6 to 831aab0 May 9, 2017

Improve manifest validation
- Improves property validation by checking for TOML array of tables.
- Adds checks for invalid fields in dependencies and overrides.
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 9, 2017

@darkowlzz hmm - looking at the TOML spec a bit, and poking at the PR, I'm confused - array of tables isn't actually the same thing as just a plain table, because you can specify them multiple times. And we don't want that to be possible - there should be exactly one allowed [metadata] section per area - whether that's at the top-level, or within each subtable (e.g. a [[dependencies]] subtable.

Related - let's also make sure a the test exercises the possibility of nesting metadata within e.g. a [[dependencies]].

@darkowlzz darkowlzz force-pushed the darkowlzz:506-add-metadata-warn-unknown-manifest-fields branch from 831aab0 to e518855 May 9, 2017

Replace metadata array of tables with just table
- Makes metadata TOML table.
- Allows metadata table in "dependencies" and "overrides".
@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 9, 2017

@sdboyer great! now that makes things more clear :)

  • metadata is now a TOML table [metadata] (map).
  • metadata can be added within subtables of [[dependencies]] and [[overrides]], only as TOML table (map). Added the tests for the same.
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 9, 2017

Awesome, this looks great! Thanks so much 😄 🎉

@sdboyer

sdboyer approved these changes May 9, 2017

@sdboyer sdboyer merged commit b3bd16e into golang:master May 9, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate no new or fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mdelder

This comment has been minimized.

Copy link

mdelder commented May 9, 2017

It appears that this PR may have triggered a regression. Consuming the latest, I see:

# github.com/golang/dep
/go/src/github.com/golang/dep/manifest.go:109: undefined: internal.Logf
# github.com/golang/dep
/go/src/github.com/golang/dep/manifest.go:109: undefined: internal.Logf
/usr/local/bin/kube-tlssec-prep.sh: line 22: dep: not found
/usr/local/bin/kube-tlssec-prep.sh: line 23: dep: not found

From:

go get github.com/golang/dep

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 9, 2017

@mdelder it did indeed - thanks for responding quickly here. I'm updating things now.

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#528 from darkowlzz/506-add-metadata-warn-un…
…known-manifest-fields

Add metadata to manifest & warn on unknown fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment