840531 - Fixes issue with inability to individually promote packages #335

ehelms · 2012-07-18T20:34:32Z

attached to a system template or changeset that have more than a single
dash in the name.

This bug fix includes the removal of adding a package to a system
template by NVRE since this not only complicates the issue but is
inconsistent. NVRE is not availabe for adding packages in the UI, and
is never used by any aspect of the system template.

@iNecas
@jlsherrill

jlsherrill · 2012-07-20T13:21:23Z

src/app/models/system_template.rb

-    if pack_attrs = Katello::PackageUtils.parse_nvrea_nvre(package_name)
-      self.packages.create!(:package_name => pack_attrs[:name], :version => pack_attrs[:version], :release => pack_attrs[:release], :epoch => pack_attrs[:epoch], :arch => pack_attrs[:arch])
-    else
+    package = Resources::Pulp::Package.name_search(package_name)


So one thing to note here is that this function search all of pulp to see if the name exists. I think what we may want to do is just search all of library for that organization (or just all of that organization).

Is there a current way to do that? I have a future enhancement planned to utilize Elastic Search to handle the searching better, but with current functionality only saw a limited name_search available given the restriction that the only piece of information is a package name.

Calling something like:

repo_ids = gather_library_repos
query = "name:%s" % Katello::Search::filter_input(name)
Glue::Pulp::Package(query, 0, 1, repo_ids)

should solve the problem. That uses elastic search to do it (there
isn't an efficient way to do that in pulp i don't believe).

On Fri, Jul 20, 2012 at 9:25 AM, Eric D Helms
reply@reply.github.com
wrote:

@@ -205,20 +205,19 @@ def export_as_tdl

def add_package package_name

if pack_attrs = Katello::PackageUtils.parse_nvrea_nvre(package_name)

self.packages.create!(:package_name => pack_attrs[:name], :version => pack_attrs[:version], :release => pack_attrs[:release], :epoch => pack_attrs[:epoch], :arch => pack_attrs[:arch])

else

package = Resources::Pulp::Package.name_search(package_name)

Is there a current way to do that? I have a future enhancement planned to utilize Elastic Search to handle the searching better, but with current functionality only saw a limited name_search available given the restriction that the only piece of information is a package name.

Reply to this email directly or view it on GitHub:
https://github.com/Katello/katello/pull/335/files#r1205223

Do we need this check at all? This changes the current behavior, that you can specify the template before the first synchronization.

Didn't think of that, but you can only do that from the API/CLI and would allow you to add packages that might not be real, or exist. Is that something we want? Both the ability to add any ole package name you want and to have that sort of inconsistency between UI and API?

I'm not a huge fan of checking at promotion time only, just because
you could potentially have a failed promotion at that time, and failed
promotions are bad (can't roll them back, etc..).

On Fri, Jul 20, 2012 at 10:41 AM, Ivan Necas
reply@reply.github.com
wrote:

@@ -205,20 +205,19 @@ def export_as_tdl

def add_package package_name

if pack_attrs = Katello::PackageUtils.parse_nvrea_nvre(package_name)

self.packages.create!(:package_name => pack_attrs[:name], :version => pack_attrs[:version], :release => pack_attrs[:release], :epoch => pack_attrs[:epoch], :arch => pack_attrs[:arch])

else

package = Resources::Pulp::Package.name_search(package_name)

I'm not talking about checking that in the promotion phase. And that should also be the time, when checking the package specified in the template, can be found in the environment. What I'm suggesting is, in this place, use only

self.packages.create!(:package_name => package_name)

without any conditions, and do the checking when promoting (or alternatively when adding the template into a changeset)

Reply to this email directly or view it on GitHub:
https://github.com/Katello/katello/pull/335/files#r1205772

I am curious about the question eric asked. Given that a template
exported to a manifest only contains a package name, is seems like the
only purpose of specifying a NVREA would be for promotion. Correct?

The generated image (from the exported template) could under most
situation have a different version than what the user supplied. Which
seems bad.

-Justin

On Fri, Jul 20, 2012 at 10:48 AM, Justin Sherrill jlsherrill@gmail.com wrote:

I'm not a huge fan of checking at promotion time only, just because
you could potentially have a failed promotion at that time, and failed
promotions are bad (can't roll them back, etc..).

On Fri, Jul 20, 2012 at 10:41 AM, Ivan Necas
reply@reply.github.com
wrote:

@@ -205,20 +205,19 @@ def export_as_tdl

def add_package package_name

if pack_attrs = Katello::PackageUtils.parse_nvrea_nvre(package_name)

self.packages.create!(:package_name => pack_attrs[:name], :version => pack_attrs[:version], :release => pack_attrs[:release], :epoch => pack_attrs[:epoch], :arch => pack_attrs[:arch])

else

package = Resources::Pulp::Package.name_search(package_name)

I'm not talking about checking that in the promotion phase. And that should also be the time, when checking the package specified in the template, can be found in the environment. What I'm suggesting is, in this place, use only

self.packages.create!(:package_name => package_name)

without any conditions, and do the checking when promoting (or alternatively when adding the template into a changeset)

Reply to this email directly or view it on GitHub:
https://github.com/Katello/katello/pull/335/files#r1205772

what about doing the check when adding the template into a changeset?

That's not bad, but if the UI was doing this and added a template to a
changset he would have to navigate to a completely different page to
fix the issue. If it was triggered when adding the package to the
template (like the ui is today) he could fix the mistake right there.

-Justin

On Fri, Jul 20, 2012 at 10:51 AM, Ivan Necas
reply@reply.github.com
wrote:

@@ -205,20 +205,19 @@ def export_as_tdl

def add_package package_name

if pack_attrs = Katello::PackageUtils.parse_nvrea_nvre(package_name)

self.packages.create!(:package_name => pack_attrs[:name], :version => pack_attrs[:version], :release => pack_attrs[:release], :epoch => pack_attrs[:epoch], :arch => pack_attrs[:arch])

else

package = Resources::Pulp::Package.name_search(package_name)

what about doing the check when adding the template into a changeset?

Reply to this email directly or view it on GitHub:
https://github.com/Katello/katello/pull/335/files#r1205883

I don't think it's a big deal, because creating a template with package, that is not yet in the environment, is more an issue for the users,that prefer the CLI (templates created thought the UI won't have that problem, because they add the packages by clicking, not by typing, right?)

So when the template was created by CLI, the fix will be probably to change the script, that was creating it, and rerun it - scripting is why the CLI is there.

iNecas · 2012-07-20T14:23:56Z

One place, where nvre is used is promoting the template: if a version for package is specified (and nvre is the only way how to do it from cli), it tries to promote the specific version of the package.

However, since it's not supported in UI and promoting packages in changeset should effectively support this, I agree that the complications it brings are bigger, then the pros and +1 for removing this.

ehelms · 2012-07-20T14:28:54Z

Maybe you can clear up something that was confusing - SystemTemplatePackage can store version, release and arch but that aspect is never included when a template is exported. Is the specification of a specific NVRE on a system template just to ensure a particular version got promoted to the next environment (as opposed to defaulting to the latest)?

iNecas · 2012-07-20T14:49:51Z

Exactly. but that was more just a plan (since it didn't get into UI). Concept of templates, as we know it today, is probably going to be abandoned, therefore I don't think it's really an issue,

iNecas · 2012-07-20T15:12:50Z

@jlsherrill I agree with you. That's why I'm for this change. But it's worth mentioning it in the changelog/errata.

mccun934 · 2012-07-20T19:17:59Z

src/app/models/system_template.rb

      self.packages.create!(:package_name => package_name)
+    else
+      raise Errors::TemplateContentException.new(_("Package with name '%s' does not exist.") % package_name)
    end
  end



since we are checking across the entirety of pulp which may be hitting different orgs, it seems this check is not accurate in that it often could be returning false positives.

I'd consider dropping the exception here if it can't find the package and just let you add it anyway. If/when we can do better validation of content we can properly warn users that the package is not available in the env.

attached to a system template or changeset that have more than a single dash in the name. This bug fix includes the removal of adding a package to a system template by NVRE since this not only complicates the issue but is inconsistent. NVRE is not availabe for adding packages in the UI, and is never used by any aspect of the system template. Removing check for package existence on add_package call since SystemTemplatePackage validation checks in the environment of the template for existence already.

ehelms · 2012-07-24T13:14:32Z

Removed check for package existence in the add_packages call due to the SystemTemplatePackage validator ensuring that a package exists inside the environment of the system template. This fix was squashed into the original commit. If there are not other issues, please ACK and merge @mccun934

jlsherrill · 2012-07-24T13:17:56Z

I'm guess I'm ok with this, I'm just unsure why it was dropped since
we can check this fully and correctly with elastic search, correct (as
we had discussed earlier)?

On Tue, Jul 24, 2012 at 9:14 AM, Eric D Helms
reply@reply.github.com
wrote:

Removed check for package existence in the add_packages call due to the SystemTemplatePackage validator ensuring that a package exists inside the environment of the system template. This fix was squashed into the original commit. If there are not other issues, please ACK and merge @mccun934

Reply to this email directly or view it on GitHub:
#335 (comment)

jlsherrill · 2012-07-24T13:20:42Z

Also considering now the Api and UI are inconsistent (the UI doesn't
allow you to add packages that don't already exist) and I believe is
already validating it with elastic search (although only at the name
level).

On Tue, Jul 24, 2012 at 9:17 AM, Justin Sherrill jlsherrill@gmail.com wrote:

I'm guess I'm ok with this, I'm just unsure why it was dropped since
we can check this fully and correctly with elastic search, correct (as
we had discussed earlier)?

On Tue, Jul 24, 2012 at 9:14 AM, Eric D Helms
reply@reply.github.com
wrote:

Removed check for package existence in the add_packages call due to the SystemTemplatePackage validator ensuring that a package exists inside the environment of the system template. This fix was squashed into the original commit. If there are not other issues, please ACK and merge @mccun934

Reply to this email directly or view it on GitHub:
#335 (comment)

ehelms · 2012-07-24T13:22:43Z

@jlsherrill

Correct in that we can use elastic search, however, the change for using elastic search was invasive to a degree. For the bug I thought I would try to find a minimal change to correct for the problem, and later via another pull request offer a re-work of that area harnessing Elastic Search.
Turns out, if you look at the validator in SystemTemplatePackage in the diff, packages that don't exist will be rejected and the piece I removed was slightly redundant. I think the check against the environment finding the package in the validator is more accurate anyhow since it does not look across Orgs.

jlsherrill · 2012-07-24T13:26:24Z

ahh, didn't realize it was redundant. Works for me!

-Justin

On Tue, Jul 24, 2012 at 9:22 AM, Eric D Helms
reply@reply.github.com
wrote:

@jlsherrill

Correct in that we can use elastic search, however, the change for using elastic search was invasive to a degree. For the bug I thought I would try to find a minimal change to correct for the problem, and later via another pull request offer a re-work of that area harnessing Elastic Search.

Turns out, if you look at the validator in SystemTemplatePackage in the diff, packages that don't exist will be rejected and the piece I removed was slightly redundant. I think the check against the environment finding the package in the validator is more accurate anyhow since it does not look across Orgs.

Reply to this email directly or view it on GitHub:
#335 (comment)

mccun934 · 2012-07-24T21:49:30Z

btw, tested this with the 1.0.1 branched code and it worked fine.

mccun934 · 2012-07-24T21:50:15Z

eric, feel free to merge. it states it can't be automatically merged so I leave it up to you

Conflicts: src/spec/models/model_spec_helper.rb

840531 - Fixes issue with inability to individually promote packages

ghost assigned mccun934 Jul 18, 2012

jlsherrill reviewed Jul 20, 2012
View reviewed changes

mccun934 reviewed Jul 20, 2012
View reviewed changes

Merge remote-tracking branch 'katello/master' into bug-840531

60da0f1

Conflicts: src/spec/models/model_spec_helper.rb

ehelms added a commit that referenced this pull request Jul 25, 2012

Merge pull request #335 from ehelms/bug-840531

afa5846

840531 - Fixes issue with inability to individually promote packages

ehelms merged commit afa5846 into Katello:master Jul 25, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

840531 - Fixes issue with inability to individually promote packages #335

840531 - Fixes issue with inability to individually promote packages #335

ehelms commented Jul 18, 2012

jlsherrill Jul 20, 2012

ehelms Jul 20, 2012

jlsherrill Jul 20, 2012

iNecas Jul 20, 2012

ehelms Jul 20, 2012

jlsherrill Jul 20, 2012

jlsherrill Jul 20, 2012

iNecas Jul 20, 2012

jlsherrill Jul 20, 2012

iNecas Jul 20, 2012

iNecas commented Jul 20, 2012

ehelms commented Jul 20, 2012

iNecas commented Jul 20, 2012

iNecas commented Jul 20, 2012

mccun934 Jul 20, 2012

ehelms commented Jul 24, 2012

jlsherrill commented Jul 24, 2012

jlsherrill commented Jul 24, 2012

ehelms commented Jul 24, 2012

jlsherrill commented Jul 24, 2012

mccun934 commented Jul 24, 2012

mccun934 commented Jul 24, 2012

840531 - Fixes issue with inability to individually promote packages #335

840531 - Fixes issue with inability to individually promote packages #335

Conversation

ehelms commented Jul 18, 2012

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

iNecas commented Jul 20, 2012

ehelms commented Jul 20, 2012

iNecas commented Jul 20, 2012

iNecas commented Jul 20, 2012

Choose a reason for hiding this comment

ehelms commented Jul 24, 2012

jlsherrill commented Jul 24, 2012

jlsherrill commented Jul 24, 2012

ehelms commented Jul 24, 2012

jlsherrill commented Jul 24, 2012

mccun934 commented Jul 24, 2012

mccun934 commented Jul 24, 2012