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

build: fix install git in container #4197

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

jeohist
Copy link
Contributor

@jeohist jeohist commented Nov 20, 2024

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

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • All new and existing tests passed.

Copy link

codesandbox-ci bot commented Nov 20, 2024

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
Copy link
Member

/cc @skycaptain ;)

@skycaptain
Copy link
Contributor

skycaptain commented Nov 20, 2024

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
Copy link
Contributor Author

jeohist commented Nov 20, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

jeohist commented Nov 20, 2024

@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
Copy link
Member

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
Copy link
Contributor Author

jeohist commented Nov 20, 2024

No worries 😄

@skycaptain
Copy link
Contributor

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.

@jeohist jeohist changed the title fix(container): install git in container ci: fix install git in container Nov 20, 2024
@escapedcat
Copy link
Member

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

With the switch to Alpine in conventional-changelog#4185, `--from-last-tag` no longer works.
This is because git is no longer available in the container.
@escapedcat escapedcat changed the title ci: fix install git in container build: fix install git in container Nov 21, 2024
@escapedcat escapedcat merged commit 3bda93d into conventional-changelog:master Nov 21, 2024
7 of 8 checks passed
@escapedcat
Copy link
Member

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants