conventional-changelog / commitlint

📓 Lint commit messages
https://commitlint.js.org
MIT License
16.93k stars 912 forks source link

build: fix install git in container #4197

Closed jeohist closed 5 days ago

jeohist commented 6 days ago

Install git in container.

Description

git is not copied from the builder layer to the final layer, so we have to install it again.

Motivation and Context

With the switch to Alpine in #4185, --from-last-tag (and possibly other arguments relying on git) no longer works. This is because git is no longer available in the container. See #4196

How Has This Been Tested?

Tested locally by using the new image in combinat

Types of changes

Checklist:

codesandbox-ci[bot] commented 6 days ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

escapedcat commented 6 days ago

/cc @skycaptain ;)

skycaptain commented 6 days ago

LGTM. Just a smaller detail, I would move the line RUN apk add --no-cache git before COPY --from=builder /src/*.tgz ./, to improve layer caching.

jeohist commented 6 days ago

LGTM. Just a smaller detail, I would move the line RUN apk add --no-cache git before COPY --from=builder /src/*.tgz ./, to improve layer caching.

Good point! I've fixed it and pushed the changes.

escapedcat commented 6 days ago

@jeohist would you mind removing the (container) scope from the commit message. It's not one of the commitlint scopes which are just the package names.

Since the last release and looking at the changelog I'm still asking myself if this really a feat/fix for commitlint or more like a chore or a ci kinda type. wdyt?

jeohist commented 6 days ago

@escapedcat Sure, amended the commit message. I think fix(ci) would work best, I don't think it's a chore (which I associate more with regular gruntwork) since it fixes broken functionality. But it also doesn't affect the core product, just one of the ways to implement it.

escapedcat commented 6 days ago

Uhm, would you mind changing it to i.e. ci: fix install git in container

The idea is that this is not fixing anything for commitlint itself. ci is not a scope as well, but it is a type.

I feel like that accepting the container change as a feat was already wrong.

Sorry for making this complicated than it should be.

jeohist commented 6 days ago

No worries 😄

skycaptain commented 6 days ago

Interesting point. Given the nature of this project, keeping a close eye on commit messages is essential 😀. I initially chose to commit the change as feat: because the documentation mentions using the image for CI and I therefore considered the image an integral part of the "product." Maybe, build: might have been a more appropriate option though.

escapedcat commented 6 days ago

@skycaptain thanks, I didn't even see build.... sorry @jeohist , please one more time and then we're good, I'll promise ;)

escapedcat commented 5 days ago

Thanks everyone!