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

Mediatype of layers should be application/vnd.oci.image.layer.v1.tar+gz #794

Merged
merged 1 commit into from
Dec 18, 2019
Merged

Mediatype of layers should be application/vnd.oci.image.layer.v1.tar+gz #794

merged 1 commit into from
Dec 18, 2019

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Nov 6, 2019

This is another thing I noticed this morning - the mediatype for layers should be in the layers family, but there were two tests (checking for other things) that had config types instead. Should this have been caught, or given that the tests were for failures for other things, it would have been caught had those other things not been wrong?

Signed-off-by: Vanessa Sochat [email protected]

@cyphar
Copy link
Member

cyphar commented Dec 3, 2019

The tests are failing because your commit's subject line is too long -- you could just make it "schema: use correct media-types for layers in tests" or something and provide the information in the body of the commit message.

Otherwise,
LGTM

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

LGTM

Approved with PullApprove

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

@cyphar pattern-matcher is not catching your approval, if you will will bump that again

@jonjohnsonjr
Copy link
Contributor

I'm not sure about this one. The spec says:

Manifests concerned with portability SHOULD use one of the above media types. An encountered mediaType that is unknown to the implementation MUST be ignored.

(the above media types being layer media types), but it doesn't forbid layer descriptors that point to config files.

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

fair, but this is just in a test case

@jonjohnsonjr
Copy link
Contributor

Yeah I could take this PR or leave it 🤷‍♂️

What's there isn't wrong, it's just weird.

@cyphar
Copy link
Member

cyphar commented Dec 17, 2019

@vbatts The tests are failing because of the commit message -- @vsoch should fix that before we merge (since we use git-validation it will cause test failures for the rest of time 😉).

@vsoch
Copy link
Contributor Author

vsoch commented Dec 17, 2019

What's wrong with the git message?

@vsoch
Copy link
Contributor Author

vsoch commented Dec 17, 2019

Just a high level observation - it's really really really hard to contribute simple things here because of these rules.

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019 via email

@cyphar
Copy link
Member

cyphar commented Dec 17, 2019

What's wrong with the git message?

The subject line (the first line of the message) is longer than 90 characters. This is the cause of the CI failure for this change.

Just a high level observation - it's really really really hard to contribute simple things here because of these rules.

I agree that these rules are frustrating (especially for drive-by contributors), but on the other hand there needs to be some limit to how long subject lines should be (because if they're too long, tools like git log start being quite hard to read in a terminal or small screen). 90 characters happens to be the limit we picked.

Now, I could also fix it for you -- but I personally don't like it when maintainers push to my tree -- so I don't do it to other people unless they've already given me permission to (annoyingly, GitHub has made "allow maintainers push to my tree" an opt-out option when creating a PR -- so I can't be sure if the person who submitted the PR actually has affirmatively given permission or if they didn't notice the option). Also, pushing to someone else's tree breaks the authorship and any signatures the author might have.

All of that being said -- we could drop using git-validation to not make this a hard requirement of PRs (personally I don't mind the length of this particular commit message). After all, we now use the DCO bot to check for Signed-off-by so we no longer need git-validation for that...

@vsoch
Copy link
Contributor Author

vsoch commented Dec 17, 2019

okay I just did a git --amend (I hope I didn't mess it up!)

@vsoch
Copy link
Contributor Author

vsoch commented Dec 17, 2019

@cyphar it's definitely an interesting conversation, because I have a colleague I was talking with recently about this, and he puts beautifully detailed (many lines) of message about the details of the work per commit for a large chunk of work. It's definitely a tradeoff between quality and quantity, and I do agree that it's probably best to put a reasonable limit and then try to make the message high quality. I am most certainly guilty of short, stupid and useless commit messages that could be better stated. :)

@vbatts
Copy link
Member

vbatts commented Dec 18, 2019

LGTM

Approved with PullApprove

@cyphar
Copy link
Member

cyphar commented Dec 18, 2019

LGTM.

Approved with PullApprove

@cyphar cyphar merged commit 8c25739 into opencontainers:master Dec 18, 2019
@cyphar
Copy link
Member

cyphar commented Dec 18, 2019

@vsoch Just to be clear, there isn't a limit on the total size of the commit message -- it's just the first line of the commit message which has a limit placed on it (in order to make it easier to reference and make git log easier to read).

Personally I found that the way the Linux kernel does git commit messages (write them as an email, with the subject line being the first line of the commit message) really helps you get a better undersanding of how to write a commit message which makes sense by itself. Unlike most projects, Linux doesn't have a formal bug tracker so all of the context for a commit is included in the message itself (as well as references to other commits).

@vsoch
Copy link
Contributor Author

vsoch commented Dec 18, 2019

hooray! Thanks everyone :)

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.

4 participants