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

feat: allow setting runners.docker.services #491

Merged

Conversation

jonmcewen
Copy link
Contributor

Description

Enable configuration of docker mirror via a service defined in config.toml

See https://gitlab.com/gitlab-org/gitlab-runner/-/issues/27171 and https://docs.gitlab.com/ee/ci/docker/using_docker_build.html#the-service-in-the-gitlab-runner-configuration-file

Migrations required

NO

Verification

Default example updated to demonstrate feature. Tested with this CI job: https://gitlab.com/jonmcewen1/yet-another-spring/-/jobs/2484859847

Documentation

We use pre-commit to update the Terraform inputs and outputs in the documentation via terraform-docs. Ensure you have installed those components.

@jonmcewen jonmcewen changed the title feat: allow setting runners.docker.services. #489 feat: allow setting runners.docker.services May 20, 2022
@npalm
Copy link
Collaborator

npalm commented May 20, 2022

@jonmcewen just did a large update on develop, the module is now bumped to AWS provider 4.x. Please can you rebase your PR? Sorry for the inconvience.

@npalm
Copy link
Collaborator

npalm commented May 20, 2022

close: #489

Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonmcewen change looks good to me. Not tested yet. Small remark.

variables.tf Show resolved Hide resolved
@jonmcewen jonmcewen force-pushed the 489-allow-setting-runners-docker-services branch from 7568d72 to f1d5f68 Compare May 23, 2022 08:28
@jonmcewen jonmcewen requested a review from npalm June 16, 2022 13:35
@jonmcewen
Copy link
Contributor Author

@npalm is there more work needed on this, or can it be merged? Thanks

@npalm
Copy link
Collaborator

npalm commented Jun 16, 2022

Sorry had holiday last week's. Will check PRs asap

@jonmcewen
Copy link
Contributor Author

Sorry had holiday last week's. Will check PRs asap

Me too :-)

Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, see remark. Need to test

examples/runner-default/jon.tfvars Outdated Show resolved Hide resolved
@jonmcewen jonmcewen requested a review from npalm August 4, 2022 12:54
@jonmcewen
Copy link
Contributor Author

Conflict resolved

@jonmcewen
Copy link
Contributor Author

@npalm please could you review and merge?

@jonmcewen
Copy link
Contributor Author

PR has status "Changes Requested" but I think I have already addressed all comments

@jonmcewen
Copy link
Contributor Author

@npalm if you have any time, please could you have another look at this PR? Is there more I need to do for this to be merged? I took your "Need to test" as you needing to test it (I already did, of course). Many thanks

@npalm
Copy link
Collaborator

npalm commented Oct 25, 2022

@npalm if you have any time, please could you have another look at this PR? Is there more I need to do for this to be merged? I took your "Need to test" as you needing to test it (I already did, of course). Many thanks

Thanks will check asap

@jonmcewen
Copy link
Contributor Author

@cinacio @kayman-mk @baolsen I'm looking for a new reviewer for this PR please. I'd really like to get this merged. Many thanks, Jon

examples/runner-default/main.tf Show resolved Hide resolved
@kayman-mk
Copy link
Collaborator

Job output from your test looks good to me. I suppose that you rebased your branch on the current development branch?

@jonmcewen
Copy link
Contributor Author

jonmcewen commented Nov 29, 2022

Job output from your test looks good to me. I suppose that you rebased your branch on the current development branch?

@kayman-mk I did rebase, but it was some time ago. Do I need to rebase again? Are you able to remove @npalm as a pending reviewer, or does the PR need more than one approval?

@kayman-mk
Copy link
Collaborator

Branch is almost up to date. Just some pending doc changes. Let me quickly test it.

@kayman-mk
Copy link
Collaborator

Succeeded. I did not activate your new feature and the pipeline is still running.

@jonmcewen
Copy link
Contributor Author

There is still a "changes requested", but I can't see what it is for. How do we clear that?

@jonmcewen
Copy link
Contributor Author

Is this now to be superseded by #511?

@kayman-mk
Copy link
Collaborator

@npalm Could you check this one please? PR indicates "changes requested" but we can't find any. I tested the PR a week ago and it looked good, at least without using the new feature.

@npalm npalm merged commit 6d73e99 into cattle-ops:develop Dec 11, 2022
This was referenced Dec 11, 2022
@jonmcewen
Copy link
Contributor Author

Many thanks @kayman-mk @npalm

@jonmcewen jonmcewen deleted the 489-allow-setting-runners-docker-services branch December 12, 2022 08:23
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.

3 participants