fixes #18964 - lifecycle environment container image name #7262

thomasmckay · 2018-03-24T15:18:59Z

This feature allows a lifecycle environment to specify a image name pattern (erb) to
use instead of the default ($org-$prod-$env-$cv-$name).

For example "<%= organization.label %>/<%= docker_upstream_name %>"

The pattern, on the lifecycle UI details page, is used for container image repositories. These repos have a name and label but in pulp they also have a "registry name". This registry name is what users use when referencing the repo for "docker pull" or "skopeo copy".

Prior to this PR, the registry name was hard coded based on a combination of organization, product, content view, life cycle, and repo labels. This style of auto-naming remains if the life cycle pattern is left blank.

The pattern is ERB format. This allows a rich flexibility in the resulting names. In the simplest, and likely the most common, is to simply use the repository's original container image name as the pattern.

<%= repository.docker_upstream_name %>

It is important to remember that the results of running a repository through a life cycle's pattern must be unique across all repos in all orgs. Thus the same pattern should be reused in multiple life cycle's with care.

Variables available in ERB:

organization.name
organization.label
repository.name
repository.label
repository.docker_upstream_name
repository.url
content_view.label
content_view.name
content_view.version
product.name
product.label
lifecycle_environment.name
lifecycle_environment.label

theforeman-bot · 2018-03-24T15:19:07Z

Issues: #18964

thomasmckay · 2018-03-24T15:25:12Z

@jlsherrill @parthaa @JacobCallahan - This feature will allow a 1:1 mirroring of a remote registry with the opportunity to keep the original image names (ie. openshift3/ose stays openshift3/ose for docker pull).

Open questions:

Should this be allowed for Library env? This would allow foreman to act as a constant mirror without user intervention, scheduled syncs would keep remote and foreman registries the same. (Note no mirror-on-sync feature in pulp @ipanova ;) )

Should I put a unique constraint on the registry_name_pattern? This will keep the chance of two repos having the same-named images. (I think that would violate pulp's rules but will confirm.)

When a registry_name_pattern is changed for a lifecycle, what is the best way to update all the component content view repos? How should this be presented to user?

How can I migrate all repos to retain their docker_upstream_name which was not being propogated to CVV repos? In a rake task or the migrate file?

thomasmckay · 2018-03-24T18:31:28Z

P.S. @parthaa - I changed the validation to allow an upstream name without an upstream url. This would let images that are hammer or docker pushed to use the upstream name in their registry pattern. (ie. disconnected registry mirroring)

ipanova · 2018-03-26T08:57:11Z

@thomasmckay it is ok to have 2 different repos with same upstream_name, just make sure the registry_id is different because of https://pulp.plan.io/issues/3165

thomasmckay · 2018-03-27T12:41:09Z

Current dilemma: A new attribute is added to lifecycle environment, registry_name_pattern. This pattern (ERB) will define the container_repository_name for all repos in that environment. At the moment, the repo attribute is only updated through a before_validation on save.

Here's how it should work: When the pattern attribute on an environment is updated, all the repos in that env need to have their name updated and propagated to pulp. All of the repos also need to be synced to capsules. (Ideally the sync would merely be for the repo attribute and not all content.)

jlsherrill · 2018-03-27T13:01:50Z

@thomasmckay so what we would do normally is make the lifecycle env create controller action async (so that a task gets returned), and as part of that action update all the repos and propagate the changes to pulp.

However that already exists and is not async. What we could do is update the environment and then spawn a task to update all the docker repositories in the environment if the attribute changed. However the user still couldn't really track that easily to wait for it to finish and go use his docker repos. I assume that a desired feature?

thomasmckay · 2018-03-27T13:49:42Z

@jlsherrill - What if on changing the env pattern the user had to explicitly kick off a resync of the component repos? I don't think this would be too burdensome since I don't see a pattern of frequent env changes being normal. Perhaps there is a new endpoint to env api to refresh repos that would return task?

houndci-bot · 2018-03-30T13:44:53Z

app/lib/actions/katello/environment/publish_repositories.rb

+        end
+
+        #???? options content_type Katello::Repository::DOCKER_TYPE
+        def plan(env, options = {})


Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.

houndci-bot · 2018-03-30T13:44:54Z

app/controllers/katello/api/v2/environments_controller.rb

      update_params[:name] = params[:environment][:new_name] if params[:environment][:new_name]
      @environment.update_attributes!(update_params)
+      if update_params[:registry_name_pattern]
+        task = sync_task(::Actions::Katello::Environment::PublishRepositories, @environment)


Useless assignment to variable - task.

houndci-bot · 2018-03-30T14:22:26Z

app/lib/actions/katello/environment/publish_repositories.rb

+        end
+
+        def run
+          env = ::Katello::KTEnvironment.find(input[:kt_environment][:id])


Useless assignment to variable - env.

thomasmckay · 2018-03-30T14:26:44Z

app/lib/actions/katello/environment/publish_repositories.rb

+              env.repositories.each do |repository|
+                repository.save!
+                plan_action(::Actions::Katello::Repository::Update, repository, {})
+                plan_action(::Actions::Pulp::Repository::Refresh, repository)


@jlsherrill @parthaa I'm just spamming actions here to try to get the the repo_registry_id to be updated in pulp. Any pointers of what to call to have that happen?

I think I have this sorted out; will clean up code a bit since I think this double action is not needed.

houndci-bot · 2018-03-31T12:05:10Z

app/lib/actions/katello/environment/publish_repositories.rb

+            concurrence do
+              env.repositories.each do |repository|
+                #repository.save!
+                plan_action(::Actions::Katello::Repository::Update, repository, {container_repository_name: repository.container_repository_name})


Redundant curly braces around a hash parameter.

houndci-bot · 2018-04-01T14:12:33Z

app/models/katello/repository.rb

+      set_container_repository_name
+      valid?( :container_repository_name) ? container_repository_name : nil
+    ensure
+      content_repository_name = original_name


Lint/UselessAssignment: Useless assignment to variable - content_repository_name. Did you mean container_repository_name?

houndci-bot · 2018-04-01T14:12:33Z

app/models/katello/repository.rb

+    def validate_container_repository_name
+      original_name = container_repository_name
+      set_container_repository_name
+      valid?( :container_repository_name) ? container_repository_name : nil


Layout/SpaceInsideParens: Space inside parentheses detected.

houndci-bot · 2018-04-01T14:12:33Z

app/models/katello/repository.rb

@@ -757,6 +756,35 @@ def self.search_by_redhat(_key, operator, value)
      end
    end

+    def safe_render_container_name(pattern=nil)


Layout/SpaceAroundEqualsInParameterDefault: Surrounding space missing in default value assignment.

houndci-bot · 2018-04-01T14:12:33Z

app/lib/actions/katello/environment/publish_repositories.rb

+        end
+
+        def run
+          env = ::Katello::KTEnvironment.find(input[:kt_environment][:id])


Lint/UselessAssignment: Useless assignment to variable - env.

houndci-bot · 2018-04-01T14:12:33Z

app/lib/actions/katello/environment/publish_repositories.rb

+          concurrence do
+            repositories.each do |repository|
+              sequence do
+                plan_action(::Actions::Katello::Repository::Update, repository, {container_repository_name: repository.container_repository_name})


Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

houndci-bot · 2018-04-01T14:12:33Z

app/controllers/katello/api/v2/environments_controller.rb

      update_params[:name] = params[:environment][:new_name] if params[:environment][:new_name]
      @environment.update_attributes!(update_params)
+      if update_params[:registry_name_pattern]
+        task = sync_task(::Actions::Katello::Environment::PublishRepositories, @environment, content_type: Katello::Repository::DOCKER_TYPE)


Lint/UselessAssignment: Useless assignment to variable - task.

thomasmckay · 2018-04-01T14:29:13Z

Added repo name verification on environment save. https://github.com/Katello/katello/pull/7262/files#diff-043f3f54ba4d6ae4333d75f582861bd3R1

thomasmckay · 2018-04-01T14:44:02Z

...o/app/assets/javascripts/bastion_katello/environments/details/views/environment-details.html

+      <dd bst-edit-textarea="environment.registry_name_pattern"
+          on-save="save(environment)"
+          readonly="denied('edit_lifecycle_environments', environment)">
+      </dd>


@waldenraines - Is there a patternfly way to add some formatted help text to a field? I want to give brief description of valid formats but it should be formatted and not just a big unreadable string.

we do this on create, but i couldn't find any examples of on edit :(

houndci-bot · 2018-04-02T01:56:52Z

app/models/katello/repository.rb

@@ -102,10 +104,19 @@ class Repository < Katello::Model
    validates :product_id, :presence => true
    validates :pulp_id, :presence => true, :uniqueness => true, :if => proc { |r| r.name.present? }
    validates :checksum_type, :inclusion => {:in => CHECKSUM_TYPES}, :allow_blank => true
+    #???? refactor these into validator class, re-use in EnvironmentDockerRepositories validator
+=begin


Style/BlockComments: Do not use block comments.

houndci-bot · 2018-04-02T01:56:52Z

app/models/katello/repository.rb

@@ -102,10 +104,19 @@ class Repository < Katello::Model
    validates :product_id, :presence => true
    validates :pulp_id, :presence => true, :uniqueness => true, :if => proc { |r| r.name.present? }
    validates :checksum_type, :inclusion => {:in => CHECKSUM_TYPES}, :allow_blank => true
+    #???? refactor these into validator class, re-use in EnvironmentDockerRepositories validator


Layout/CommentIndentation: Incorrect indentation detected (column 4 instead of 0).

houndci-bot · 2018-04-02T01:56:52Z

app/lib/katello/validators/environment_docker_repositories_validator.rb

+          name = repository.safe_render_container_name(environment.registry_name_pattern)
+          #???? check pre-render name is same as post-render?
+
+          if !ContainerImageNameValidator.validate_name(name)


Style/NegatedIf: Favor unless over if for negative conditions.

houndci-bot · 2018-04-03T17:13:41Z

test/controllers/api/v2/environments_controller_test.rb

+    end
+
+    def test_update_pattern_fail
+      original_label = @staging.label


Lint/UselessAssignment: Useless assignment to variable - original_label.

houndci-bot · 2018-04-03T17:13:41Z

test/controllers/api/v2/environments_controller_test.rb

@@ -79,6 +105,49 @@ def test_update
      assert_equal original_label, @staging.label
    end

+    def test_update


Lint/DuplicateMethods: Method Katello::Api::V2::EnvironmentsControllerTest#test_update is defined at both test/controllers/api/v2/environments_controller_test.rb:93 and test/controllers/api/v2/environments_controller_test.rb:108.

jlsherrill · 2018-04-03T18:05:52Z

app/controllers/katello/api/v2/environments_controller.rb

      update_params[:name] = params[:environment][:new_name] if params[:environment][:new_name]
      @environment.update_attributes!(update_params)
+      if update_params[:registry_name_pattern]
+        #???? should task be returned? what would be returned when async_task not called?
+        #???? optional param to make this a sync_task?


I think i would return the task here. Technically the env is updated, its just further work that can be tracked

@jlsherrill Should I change this to call update_attributes! in the plan() of the task so that there is always a task to return? (Is plan() the right place to do that?

theforeman-bot · 2018-04-04T12:51:54Z

There were the following issues with the commit message:

commit message for 5c6ed65 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

thomasmckay · 2018-04-04T13:44:29Z

State of the PR: All functionality works and there is robust checking of environment registry pattern to prevent duplicate repository names. Unit test writing continues but live usage testing by others would be appreciated.

theforeman-bot · 2018-05-04T18:17:35Z

There were the following issues with the commit message:

a05eb81 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

thomasmckay · 2018-05-07T15:45:43Z

app/lib/katello/validators/environment_docker_repositories_validator.rb

+        end
+
+        Katello::Repository.where(content_type: Katello::Repository::DOCKER_TYPE).each do |repository|
+          if !repositories.include?(repository) && names.include?(repository.container_repository_name)


@jlsherrill - Updated this with original intention: Check all repos except for the current ones being processed (they're checked above) for name clashes.

Really struggling to write sensible tests.

thomasmckay · 2018-05-07T21:33:08Z

Test I'd like to write in FactoryBot:

CV 1 w/ repo A
Add CV to LCE X
Set LCE pattern "zzzzz" (static)
Assert success
CV 2 w/ repo A
Add CV to LCE X
Assert error

houndci-bot · 2018-05-08T15:02:53Z

test/models/content_view_test.rb

+      repo2_cv = build(:docker_repository, product: product, content_view_version: @organization.default_content_view.versions.first, library_instance_id: repo2_lib.id)
+      version2 = create(:katello_content_view_version, :content_view => view2, :repositories => [repo2_cv])
+
+


Layout/EmptyLines: Extra blank line detected.

jlsherrill · 2018-05-08T18:16:42Z

[test katello]

thomasmckay · 2018-05-09T00:44:27Z

[test katello]

thomasmckay · 2018-05-09T17:42:45Z

Test written. Updated a few existing tests that had failed w/ the registry name unique constraint.
https://github.com/Katello/katello/pull/7262/files#diff-2b24dd48e0c60620fb05b0e0f6a83e66R19

jlsherrill · 2018-05-15T14:28:58Z

[test katello]

jlsherrill · 2018-05-16T14:32:10Z

Small rubocop error.

Also, i seem to be unable to repromote an existing cv that is already in dev, i get "Content View publish to environment dev will result in invalid container image name of member repositories", even though i'm promoting a cv thats already there.

thomasmckay · 2018-05-17T14:03:13Z

@jlsherrill - Removed the repos check and switched to rely on uniqueness validator for repos. I am less excited about this but I can't wrap my head around how I could correctly check all the names. :)

jlsherrill · 2018-05-18T01:50:03Z

app/models/katello/repository.rb

+        allowed_methods = {}
+        allowed_vars = {}
+        scope_variables = {repository: repository, organization: repository.organization, product: repository.product,
+                           environment: repository.environment, contentview: repository.content_view_version.content_view}


mind making this 'content_view' instead of 'contentview'?

thomasmckay · 2018-05-21T13:47:32Z

[test katello]

theforeman-bot added Needs testing Not yet reviewed labels Mar 24, 2018

thomasmckay force-pushed the lifecycle-docker branch from a523e9b to e272233 Compare March 27, 2018 11:50

thomasmckay force-pushed the lifecycle-docker branch from e272233 to 4f2bba3 Compare March 30, 2018 13:44

houndci-bot reviewed Mar 30, 2018

View reviewed changes

thomasmckay commented Mar 30, 2018

View reviewed changes

houndci-bot reviewed Mar 31, 2018

View reviewed changes

thomasmckay force-pushed the lifecycle-docker branch from 9405a81 to 049b9df Compare April 1, 2018 14:12

houndci-bot reviewed Apr 1, 2018

View reviewed changes

thomasmckay commented Apr 1, 2018

View reviewed changes

houndci-bot reviewed Apr 2, 2018

View reviewed changes

thomasmckay force-pushed the lifecycle-docker branch from f22c17e to f74a688 Compare April 2, 2018 17:37

houndci-bot reviewed Apr 3, 2018

View reviewed changes

jlsherrill reviewed Apr 3, 2018

View reviewed changes

thomasmckay changed the title ~~[WIP] fixes #18964 - lifecycle environment container image name~~ fixes #18964 - lifecycle environment container image name Apr 4, 2018

thomasmckay force-pushed the lifecycle-docker branch from 7cb41c0 to 5c6ed65 Compare April 4, 2018 12:51

theforeman-bot added the Waiting on contributor label Apr 4, 2018

thomasmckay force-pushed the lifecycle-docker branch from 5c6ed65 to d42a289 Compare April 6, 2018 01:12

theforeman-bot removed the Waiting on contributor label Apr 6, 2018

thomasmckay force-pushed the lifecycle-docker branch from cd16ec3 to a05eb81 Compare May 4, 2018 18:17

theforeman-bot added the Waiting on contributor label May 4, 2018

thomasmckay force-pushed the lifecycle-docker branch from a05eb81 to dd6fe8a Compare May 7, 2018 15:31

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 7, 2018

thomasmckay commented May 7, 2018

View reviewed changes

thomasmckay force-pushed the lifecycle-docker branch from dd6fe8a to 4d69a3b Compare May 8, 2018 15:02

houndci-bot reviewed May 8, 2018

View reviewed changes

thomasmckay force-pushed the lifecycle-docker branch from 4d69a3b to e08b81e Compare May 8, 2018 17:30

thomasmckay force-pushed the lifecycle-docker branch from e08b81e to 0d8157b Compare May 8, 2018 19:59

thomasmckay force-pushed the lifecycle-docker branch from 0d8157b to f55ffbd Compare May 9, 2018 14:30

thomasmckay force-pushed the lifecycle-docker branch 2 times, most recently from bcbad9f to 3ba1341 Compare May 16, 2018 22:38

jlsherrill reviewed May 18, 2018

View reviewed changes

thomasmckay force-pushed the lifecycle-docker branch from 3ba1341 to 1243761 Compare May 18, 2018 18:49

fixes #18964 - lifecycle environment container image name

060c658

thomasmckay force-pushed the lifecycle-docker branch from 1243761 to 060c658 Compare May 18, 2018 19:57

jlsherrill approved these changes May 21, 2018

View reviewed changes

thomasmckay merged commit 961148c into Katello:master May 21, 2018

thomasmckay mentioned this pull request May 22, 2018

refs #18964 - registry name pattern Katello/hammer-cli-katello#558

Closed

		repo2_cv = build(:docker_repository, product: product, content_view_version: @organization.default_content_view.versions.first, library_instance_id: repo2_lib.id)
		version2 = create(:katello_content_view_version, :content_view => view2, :repositories => [repo2_cv])

fixes #18964 - lifecycle environment container image name #7262

fixes #18964 - lifecycle environment container image name #7262

Conversation

thomasmckay commented Mar 24, 2018 • edited

theforeman-bot commented Mar 24, 2018

thomasmckay commented Mar 24, 2018 • edited

thomasmckay commented Mar 24, 2018

ipanova commented Mar 26, 2018

thomasmckay commented Mar 27, 2018

jlsherrill commented Mar 27, 2018

thomasmckay commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasmckay commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Apr 4, 2018

thomasmckay commented Apr 4, 2018

theforeman-bot commented May 4, 2018

Choose a reason for hiding this comment

thomasmckay commented May 7, 2018

Choose a reason for hiding this comment

jlsherrill commented May 8, 2018

thomasmckay commented May 9, 2018

thomasmckay commented May 9, 2018

jlsherrill commented May 15, 2018

jlsherrill commented May 16, 2018

thomasmckay commented May 17, 2018

Choose a reason for hiding this comment

thomasmckay commented May 21, 2018

thomasmckay commented Mar 24, 2018 •

edited

thomasmckay commented Mar 24, 2018 •

edited