voxpupuli / metadata-json-lint

Tool to check the validity of Puppet metadata.json files
Apache License 2.0
29 stars 27 forks source link

author is not a required field #26

Closed 3flex closed 6 years ago

3flex commented 8 years ago

It's marked required at https://github.com/voxpupuli/metadata-json-lint/blob/5a4c3a1aac746096b047db7494bf29b4718d6a76/lib/metadata_json_lint.rb#L60

but is not required per https://docs.puppet.com/puppet/latest/reference/modules_metadata.html#allowed-keys-in-metadatajson

bastelfreak commented 8 years ago

I'm not 100% sure here. But I think we once tried to release to the forge without author and that failed. So I would call it mandatory.

3flex commented 8 years ago

https://docs.puppet.com/puppet/latest/reference/modules_publishing.html#build-your-module contradicts the other doc I linked to above, saying it actually is required... I'm going to close this.

3flex commented 8 years ago

Reopening, as I did some testing with uploads to the Forge. The only truly mandatory fields to upload something to the Forge are:

Author, license, dependencies are not mandatory, and you can upload without them.

If missing, license defaults to Apache 2.0. This should remain mandatory in the linter though since if missing you might upload with a license that wasn't intended.

Author is interesting - when set, it seems to be ignored by the Forge. The Forge uses your Forge account's Display Name as the author name. You can see this on https://forge.puppet.com/puppetlabs/puppetdb, where Author is shown as "Puppet", but if you download the latest release and check metadata.json it shows author is "puppetlabs".

Dependencies can obviously be missing if the module doesn't have any, though the docs say it's a mandatory key so it can be left in.

So mandating "Author" is actually not too helpful since it's ignored in the Forge anyway. You'll only see it if looking at metadata source itself.

Feel free to close again if you'd prefer status quo, and I'll drop it :)

ghoneycutt commented 7 years ago

We should go by the documentation and not how the Forge behaves. According to the docs, it is allowed but not required.

ghoneycutt commented 7 years ago

CC: @rnelson0

rnelson0 commented 7 years ago

I guess the question is, is this correct, or is this? I'll see if anyone on Slack knows.

ghoneycutt commented 7 years ago

lulz. Nice catch :)

rnelson0 commented 7 years ago

Created DOCUMENT-605 to track this issue.

3flex commented 7 years ago

Docs at Puppet were updated to say it's required, but "If absent, this key will default to the username portion of name." - https://docs.puppet.com/puppet/latest/modules_metadata.html#allowed-keys-in-metadatajson

Name is mandatory, and must be in format "username-module", so it's not required to have author in the metadata.json because it would default to username. It's not required to be a valid metadata file, and this is now supported clearly by the documentation.

Thoughts?

rnelson0 commented 7 years ago

The lazy person in me says we should make it required and throw an error if not. But I think the most correct way is to throw a warning if it is not present and elevate it to error if the right strict flag is present.

ghoneycutt commented 7 years ago

I think the 'if absent' was done before author was marked as required and denotes their implementation. +1 this should throw something. I like @rnelson0 's approach

bmjen commented 7 years ago

@rnelson0 this issue has been fixed on the Puppet docs side, with the author field being marked required. Can this be closed?

rnelson0 commented 7 years ago

I think we need some consensus on how this should be handled by mdl, then review the code to ensure that is what happens, before we close it.

3flex commented 6 years ago

If https://github.com/voxpupuli/metadata-json-lint/issues/92 is implemented, presumably it would use the JSON Schema published on the Forge API site?

In that schema, author is required.

ekohl commented 6 years ago

Even without implementing #92 then we can consider to following the one published on the forge. With that in mind, we're already doing so and I think this issue can be closed.