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

fix repositories missing the edition name #271

Merged
merged 4 commits into from
Sep 22, 2018

Conversation

tequeter
Copy link
Contributor

The current master branch would generate a gitlab_official_ repository with an invalid URL. Basically, all edition names (ce/ee) were missing in the repository configuration.

The root of the issue was the attempt at accessing the Puppet catalog's gitlab::edition from Hiera. This can't work AFAIK unless users are instructed to only configure the classes through Hiera (undesirable IMO).

I fixed it by providing both CE/EE definitions in Hiera and adding the ensure attribute in Puppet to match the requested edition. By managing both repositories, this also ensures that the user cannot end up with a leftover. NB: the Puppet data manipulation step is uglier than it should be, by lack of a Hash -> Hash map function.

The first commit documents the bug, should you prefer a different solution.

The URL is already supposed to be set through Hiera.
This fixes repositories with missing edition string in the name and URL (ie.
`gitlab_official_` instead of `gitlab_official_ce`).
baseurl => "https://packages.gitlab.com/gitlab/gitlab-${_edition}/el/${facts['os']['release']['major']}/\$basearch",
},
},
if $_edition == 'disabled' {
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole batch of code was planned to be removed prior to the 3.0 release. It's only in here so that those who use the master in their environments will have a deprecation period.

This isn't a review, just a comment FYI. I probably should have left a comment to this effect in the code when this was refactored.

@LongLiveCHIEF
Copy link
Contributor

Can you open this as an issue? Adding a data/common.yaml with a gitlab::edition: key will solve this problem, and allow us to remove all the code you refactored entirely.

@tequeter
Copy link
Contributor Author

I created bug #273 as requested.

I don't understand your planned fix, though. Would you mind detailing it?

AFAIK as long as you %{lookup()} a value from Hiera, you can't access class parameters set in the Puppet code (typically in the profile). I wrote a failing testcase illustrating this.

Are you implying that the users of the upcoming 3.0 version should stop configuring the gitlab class from Puppet and set some/all parameters through Hiera exclusively? I would find this inconvenient.

@LongLiveCHIEF
Copy link
Contributor

AFAIK as long as you %{lookup()} a value from Hiera, you can't access class parameters set in the Puppet code (typically in the profile). I wrote a failing testcase illustrating this.

I don't know what I was thinking when I wrote that lookup in there, as that's not even how hiera works.

Are you implying that the users of the upcoming 3.0 version should stop configuring the gitlab class from Puppet and set some/all parameters through Hiera exclusively? I would find this inconvenient.

That was never my intent, and I don't even think this could be done.

I need a bit to think through what I was planning on having this file look like for the 3.0 release compared to this PR. Originally, I was going to remove the entire block coercing edition into the values we want, and it was going to look like this:

  if $manage_omnibus_repository {
    # common attributes for all repository configuration resources
    # ensures correct ordering regardless of the number or configuration
    # of repository related resources
    $resource_defaults = {
      tag    => 'gitlab_omnibus_repository_resource',
      before => Class['gitlab::install'],
    }

    # create all the repository resources
    $_repository_configuration.each() | String $resource_type, Hash $resources | {
      create_resources($resource_type, $resources, $resource_defaults)
    }
  }

That was going to be the extent of this manifest when I had planned it for the v3 release. Your PR though has a few things I want to look at in conjunction with the manifests/install.pp and the manage_upstream_edition class parameter that was meant to replace the edition param.

@LongLiveCHIEF LongLiveCHIEF merged commit 936a433 into voxpupuli:master Sep 22, 2018
tequeter added a commit to tequeter/puppet-gitlab that referenced this pull request Oct 17, 2018
Somehow, after the voxpupuli#271 and voxpupuli#272 merges we ended up without the GPG key
change for gitlab-ce and with %{lookup()} calls left in gitlab-ee. This
commit fixes the data and adds tests for it.
bastelfreak added a commit that referenced this pull request Oct 17, 2018
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.

2 participants