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

Added the option to enable Gitlab Container Registry #77639

Open
wants to merge 11 commits into
base: master
from

Conversation

@thilobillerbeck
Copy link

@thilobillerbeck thilobillerbeck commented Jan 13, 2020

Motivation for this change

In the current state, it is not possible to easily use the Gitlab Container-Registry on NixOS like the rest of Gitlab. Documentation on how to configure it is very rare. To ease the configuration for future Gitlab users on NixOS I implemented most of the required options and serivces into the Gitlab service. The only thing that has to be done manually is the root certificate generation.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)

  • Built on platform(s)

    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)

  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"

  • Tested execution of all binary files (usually in ./result/bin/)

  • Determined the impact on package closure size (by running nix path-info -S before and after)

  • Ensured that relevant documentation is up to date

  • Fits CONTRIBUTING.md.

  • Not mentioned in the template: I tested this inside a VM.

to pointing to cert like in rest of package
@aanderse
Copy link
Contributor

@aanderse aanderse commented Jan 15, 2020

@flokli unfortunately my only gitlab instance is on Debian... and I don't have anything docker related on it. This PR is entirely out of my scope for review, sorry!

@aanderse aanderse removed their request for review Jan 15, 2020
@flokli
Copy link
Contributor

@flokli flokli commented Jan 19, 2020

Sorry for the delay, I hope to take a look at this till the end of the upcoming week.

Copy link
Contributor

@talyz talyz left a comment

First of all, thank you for putting in the work and sorry for the delay! I haven't had time to try this out, but I've left some comments on things I would like to see fixed. In addition to these fixes, could you squash some of your commits? Everything, except maybe the maintainer-list addition, might as well be the same commit.

host = mkOption {
type = types.str;
default = "registry." + config.networking.hostName;
description = "Gitlab-Contaier Registry host name.";

This comment has been minimized.

@talyz

talyz Jan 23, 2020
Contributor

Minor spelling and style nitpicks. Applies to the other descriptions as well.

Suggested change
description = "Gitlab-Contaier Registry host name.";
description = "GitLab container registry host name.";
};
host = mkOption {
type = types.str;
default = "registry." + config.networking.hostName;

This comment has been minimized.

@talyz

talyz Jan 23, 2020
Contributor

Maybe host should default to services.gitlab.host, since it's running on the same machine?

@@ -633,6 +669,22 @@ in {
# Use postfix to send out mails.
services.postfix.enable = mkDefault true;

# Enable Docker Registry, if GitLab-Container Registry is enabled
services.dockerRegistry = {

This comment has been minimized.

@talyz

talyz Jan 23, 2020
Contributor

We don't want any of this to apply unless services.gitlab.registry.enable is set to true.

Suggested change
services.dockerRegistry = {
services.dockerRegistry = optionalAttrs cfg.registry.enable {
@thilobillerbeck
Copy link
Author

@thilobillerbeck thilobillerbeck commented Jan 25, 2020

I applied the suggestions from above. The squash is still to be done, but maybe you can re-review my changes in the meantime.

nixos/modules/services/misc/gitlab.nix Outdated Show resolved Hide resolved
host = mkOption {
type = types.str;
default = config.services.gitlab.host;
description = "GitLab container registry host name.";
};
port = mkOption {
type = types.int;
default = 4567;
description = "GitLab container registry port.";
};
certFile = mkOption {
type = types.path;
default = null;
description = "Path to gitLab container registry certificate.";
};
keyFile = mkOption {
type = types.path;
default = null;
description = "Path to gitLab container registry certificate-key.";
};
Comment on lines +398 to +417

This comment has been minimized.

@flokli

flokli Feb 2, 2020
Contributor

see my note from above regarding duplication of configuration options.

Options not existing there but set in the gitlab module should probably be added to the dockerRegistry module.

This comment has been minimized.

@thilobillerbeck

thilobillerbeck Feb 9, 2020
Author

These are the options, which control the endpoint, which is presented to the user. I think this should stay there.

nixos/modules/services/misc/gitlab.nix Outdated Show resolved Hide resolved
port = 5000;
enableDelete = true; # This must be true, otherwise GitLab won't manage it correctly
extraConfig = {
REGISTRY_LOG_LEVEL = "info";

This comment has been minimized.

@flokli

flokli Feb 2, 2020
Contributor

is this not the default?

This comment has been minimized.

@thilobillerbeck

thilobillerbeck Feb 9, 2020
Author

Deletion is not the default. When disabled, GitLab will throw errors, because it cannot fully delete projects.

This comment has been minimized.

@flokli

flokli Feb 9, 2020
Contributor

I was referring to the log level, at that line.

nixos/modules/services/misc/gitlab.nix Outdated Show resolved Hide resolved
@@ -633,6 +684,19 @@ in {
# Use postfix to send out mails.
services.postfix.enable = mkDefault true;

# Enable Docker Registry, if GitLab-Container Registry is enabled
services.dockerRegistry = optionalAttrs cfg.registry.enable {
enable = cfg.registry.enable;

This comment has been minimized.

@flokli

flokli Feb 9, 2020
Contributor

this can be true, as the whole attrset is inside a optionalAttrs cfg.registry.enable.

@flokli
Copy link
Contributor

@flokli flokli commented Feb 9, 2020

Can you add registry usage to the VM test, so people see how to use it, and we make sure it works (well, at least ensure the registry service starts successfully)?

@thilobillerbeck thilobillerbeck requested a review from flokli Feb 10, 2020
Copy link
Contributor

@flokli flokli left a comment

This needs a rebase on top of master and some update to reduce spam in the tests, but apart from that, LGTM!

@@ -32,6 +32,94 @@ import ./make-test-python.nix ({ pkgs, lib, ...} : with lib; {
databasePasswordFile = pkgs.writeText "dbPassword" "xo0daiF4";
initialRootPasswordFile = pkgs.writeText "rootPassword" initialRootPassword;
smtp.enable = true;
registry = {
enable = true;
certFile = pkgs.writeText "registry.crt" ''

This comment has been minimized.

@flokli

flokli Apr 7, 2020
Contributor

can you create certs on the fly, like in taskserver.nix? This should be way more readable than dumping ~80lines here ;-)

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.