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

provide ci and container image #3431

Merged
merged 1 commit into from
Sep 25, 2023
Merged

provide ci and container image #3431

merged 1 commit into from
Sep 25, 2023

Conversation

kaiehrhardt
Copy link
Contributor

@kaiehrhardt kaiehrhardt commented Dec 6, 2022

Description

TODOs:

Maintainer TODO:

  • add service acc credentials for ghcr to circleci ($USER $PASS)

Motivation and Context

#3423

Usage examples

please refer docs

How Has This Been Tested?

ci test run https://app.circleci.com/pipelines/github/kaiehrhardt/commitlint

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@kaiehrhardt kaiehrhardt marked this pull request as ready for review December 8, 2022 00:24
@kaiehrhardt
Copy link
Contributor Author

@AdeAttwood @escapedcat @shanduur

@kaiehrhardt
Copy link
Contributor Author

What else do i need to get into production?

@AdeAttwood
Copy link
Member

Thanks for this, and sorry it's take me so long to get to it. I can defo pick this up and finish off all the internal commitlint stuff. I have created a docker hub account to push this to and set up the circle ci our end. I do have some queries about the docker file.

  1. We should be using node:18-buster as the base image. This has a newer version of git installed in it that is used for trailers in some rules.

  2. There needs to be a lot more packages built than @commitlint/cli and @commitlint/config-conventional. If we only package and install these, then all the decencies of @commitlint/cli get installed from npm. This will cause issues with packages like @commitlint/types being out of sync with the CLI package.

  3. Do we really want to me setting the entrypoint of the image. I think this will only cause issues when trying to use this in CI environment. I think it will also need to be reset in GitHub action, like you have done in the docs.

This is the Dockerfile I have been looking at, heavily inspired by yours.

Dockerfile
FROM node:18-buster AS builder

WORKDIR /src

COPY . ./

RUN yarn install && yarn build

# Commit lint CLI packages
RUN npm pack @commitlint/cli
RUN npm pack @commitlint/config-validator
RUN npm pack @commitlint/ensure
RUN npm pack @commitlint/execute-rule
RUN npm pack @commitlint/format
RUN npm pack @commitlint/is-ignored
RUN npm pack @commitlint/lint
RUN npm pack @commitlint/load
RUN npm pack @commitlint/message
RUN npm pack @commitlint/parse
RUN npm pack @commitlint/read
RUN npm pack @commitlint/resolve-extends
RUN npm pack @commitlint/rules
RUN npm pack @commitlint/to-lines
RUN npm pack @commitlint/top-level
RUN npm pack @commitlint/types

# Default commitlint config
RUN npm pack @commitlint/config-conventional

FROM node:18-buster

COPY --from=builder /src/*.tgz ./

RUN npm install -g *.tgz

RUN rm ./*.tgz

I think the next steps for this is for me to take this branch and bring it under the commitlint repo. This way, we will be able to use the docker credentials to test pushing on a separate branch. Is that OK with you?

@kaiehrhardt
Copy link
Contributor Author

Same here, sorry for my late response.

I have created a docker hub account to push this to and set up the circle ci our end.

Why, just use ghcr like i did?!

We should be using node:18-buster as the base image. This has a newer version of git installed in it that is used for trailers in some rules.

Okey.

There needs to be a lot more packages built than @commitlint/cli and @commitlint/config-conventional. If we only package and install these, then all the decencies of @commitlint/cli get installed from npm. This will cause issues with packages like @commitlint/types being out of sync with the CLI package.

I will add these changes.

Do we really want to me setting the entrypoint of the image. I think this will only cause issues when trying to use this in CI environment. I think it will also need to be reset in GitHub action, like you have done in the docs.

https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#entrypoint

I think the next steps for this is for me to take this branch and bring it under the commitlint repo. This way, we will be able to use the docker credentials to test pushing on a separate branch. Is that OK with you?

Sure, but first i will incorporate your review comments.

@kaiehrhardt
Copy link
Contributor Author

Sure, but first i will incorporate your review comments.

Done.

@AdeAttwood
Copy link
Member

This is cool, I am still not sure about using GCP for container hosting. I always thought that you needed to pay for there container hosting.

@shanduur
Copy link
Contributor

shanduur commented Feb 16, 2023

@AdeAttwood GHCR is GitHub Container Registry, not GCR (GCP Container Registry).

@AdeAttwood
Copy link
Member

Really sorry, my bad. I have had a go at setting this up and have hit a couple of issues.

  1. I do not have the permissions to make the container package public on the conventional-changelog org this needs to be done by the organization administrators. @escapedcat don't know if yo can help with this.

  2. I can't seem to create an access token only targeting the commitlint packages. If I create a token, it will be able to publish packages to any GitHub package repo I have access to. Unfortunately, I don't think I will be able to use one of my tokens.

@kaiehrhardt
Copy link
Contributor Author

I do not have the permissions to make the container package public on the conventional-changelog org this needs to be done by the organization administrators. @escapedcat don't know if yo can help with this.

We definitely need help from a org maintainer.

I can't seem to create an access token only targeting the commitlint packages. If I create a token, it will be able to publish packages to any GitHub package repo I have access to. Unfortunately, I don't think I will be able to use one of my tokens.

https://docs.github.com/en/get-started/learning-about-github/types-of-github-accounts#personal-accounts

Tip: Personal accounts are intended for humans, but you can create accounts to automate activity on GitHub. This type of account is called a machine user. For example, you can create a machine user account to automate continuous integration (CI) workflows.

@kaiehrhardt
Copy link
Contributor Author

so you guys go now with github actions? #3549

i could migrate the mr, if you're still interested.

@escapedcat
Copy link
Member

Thanks @kaiehrhardt , tbh I'm as bit lost when it comes to permissions and contacting org maintainers.
Would it be possible to open an issue with requirements/what is needed and we try to tag a maintainer?

@kaiehrhardt
Copy link
Contributor Author

kaiehrhardt commented Mar 30, 2023

Thanks @kaiehrhardt , tbh I'm as bit lost when it comes to permissions and contacting org maintainers. Would it be possible to open an issue with requirements/what is needed and we try to tag a maintainer?

We don't need any special creds, because it's build-in.

Changes are done. Tags look like this.

Test: https://github.com/kaiehrhardt/commitlint/actions/runs/4564227536

@kaiehrhardt
Copy link
Contributor Author

ping @escapedcat

1 similar comment
@kaiehrhardt
Copy link
Contributor Author

ping @escapedcat

@escapedcat
Copy link
Member

Sorry @kaiehrhardt
In general I guess we can merge this. Maybe @AdeAttwood can give a 👍 .

@kaiehrhardt would you be able to maintain this in the future? I.e. if something like this needs tp be updated?

@shanduur
Copy link
Contributor

@escapedcat @kaiehrhardt this probably should be parametrized using ARG, and set inside CI instead of being hardcoded in Dockerfile.

@kaiehrhardt
Copy link
Contributor Author

@escapedcat @kaiehrhardt this probably should be parametrized using ARG, and set inside CI instead of being hardcoded in Dockerfile.

nah hardcoded is ok -> fixed with dep automation https://github.com/conventional-changelog/commitlint/blob/2b5def1a2eb84053f13d2aae4b59d2d348ff3dd9/.github/dependabot.yml

Sorry @kaiehrhardt In general I guess we can merge this. Maybe @AdeAttwood can give a 👍 .

@kaiehrhardt would you be able to maintain this in the future? I.e. if something like this needs tp be updated?

Just tag me, if you need help in a issue or pr.

Maybe add some CODEOWNERS (https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners) and add me there.

@kaiehrhardt
Copy link
Contributor Author

hm, maybe we don't need dependabot, since you use renovate https://github.com/conventional-changelog/commitlint/blob/master/package.json#L41-L56. Renovate should autodetect the Dockerfile.

@AdeAttwood
Copy link
Member

Hi all, everything looks cool with this and I can't see any reason this cant be merged, the only thing I am concert about with this is host on GitHub and being actually able to use it. I pushed the hello world image to ghcr.io/conventional-changelog/commitlint:latest can someone have a quick test someone other than me or @escapedcat can pull this image.

@kaiehrhardt
Copy link
Contributor Author

kaiehrhardt commented Apr 17, 2023

Error response from daemon: denied

https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-container-registry#pushing-container-images

When you first publish a package, the default visibility is private. To change the visibility or set access permissions, see "Configuring a package's access control and visibility." You can link a published package to a repository using the user interface or command line. For more information, see "Connecting a repository to a package."

@AdeAttwood
Copy link
Member

@kaiehrhardt this is what I tried before #3431 (comment), I don't know how we can get hold of someone who can @escapedcat any ideas.

@kaiehrhardt
Copy link
Contributor Author

kaiehrhardt commented Apr 17, 2023

we need this :/

grafik

org -> settings -> packages

or just go with dockerhub? could you add github action secrets?

@kaiehrhardt
Copy link
Contributor Author

Ping

@escapedcat
Copy link
Member

Hey @kaiehrhardt , sorry, nothing new from my side... I wrote bcoe because he might be able able to help with the org settings but no reply so far. Do we have any alternatives?

@kaiehrhardt
Copy link
Contributor Author

Hey @escapedcat

or just go with dockerhub? could you add github action secrets?

@escapedcat
Copy link
Member

Coule be done here I guess:
https://github.com/conventional-changelog/commitlint/settings/secrets/actions

Let's try this? What should I add there?

@kaiehrhardt
Copy link
Contributor Author

kaiehrhardt commented May 4, 2023

dockerhub username and token

        uses: docker/login-action@v2
        with:
          username: ${{ secrets.DOCKERHUB_USERNAME }}
          password: ${{ secrets.DOCKERHUB_TOKEN }}

@escapedcat
Copy link
Member

@AdeAttwood I guess you created this, right?: https://hub.docker.com/u/commitlint
Wanna add the creds to the settings?

@kaiehrhardt
Copy link
Contributor Author

@AdeAttwood @escapedcat any news here?

@escapedcat
Copy link
Member

So far not :(
Was hoping that Ade created that docker-account and can add the creds.

@kaiehrhardt
Copy link
Contributor Author

@escapedcat should we go with a new account to get this finally merged?

@escapedcat
Copy link
Member

Yeah, will create a new one and get back to you
/cc @AdeAttwood

.github/workflows/container-build.yml Outdated Show resolved Hide resolved
@escapedcat
Copy link
Member

@AdeAttwood please merge when you're good with this
@kaiehrhardt sorry it took so long

@AdeAttwood AdeAttwood merged commit c538d3c into conventional-changelog:master Sep 25, 2023
@kaiehrhardt kaiehrhardt deleted the dockerfile-ci branch September 25, 2023 15:00
@escapedcat
Copy link
Member

image

Looks like images from rennovate PRs are being created? Do we want/need this?

@shanduur
Copy link
Contributor

shanduur commented Oct 2, 2023

No, why is building form EVERY tag and EVERY branch? This should be built only for releases and “nightly” master builds.

@shanduur
Copy link
Contributor

shanduur commented Oct 3, 2023

Added #3674 to fix that.

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.

4 participants