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

Fixes #36310 - Migrate off ContentViews generated for repository export #11144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

parthaa
Copy link
Contributor

@parthaa parthaa commented Sep 16, 2024

Ensures no Hosts remain on ContentViews generated for repository export. If doing so would leave the Host without a valid ContentView, the Default Organization View is assigned to it.

Also implements additional filtering in the candlepin proxies controller, and several additional tests.

What considerations are taken in implementing this pull request?

At least some validation was already done with c33da7a

What are the testing steps for this pull request?

  1. Export a Content View
  2. Register host
  3. Try assigning some hosts to the content view created for export
>> host = Host.find(...)
>> cve = ::Katello::ContentViewEnvironment.where(content_view: ::Katello::ContentView.generated_for_repository_export).first
>> cf = host.content_facet
>> cf.content_view_environment_content_facets.create!(content_view_environment: cve) 
>> cf.content_view_environment_content_facets 
# verify your cve is there.
  1. Apply patch

  2. Run migration

  3. Verify that your cve got deleted from the host

>> host = Host.find(...)
>> cf = host.content_facet
>> cf.content_view_environment_content_facets 
# verify your cve is NOT there.

Copy link
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in updating the other PR. Your changes mostly look good to me... could you explain the benefit of using the FakeCVECF class here?

@parthaa
Copy link
Contributor Author

parthaa commented Sep 16, 2024

Sorry for the delay in updating the other PR. Your changes mostly look good to me... could you explain the benefit of using the FakeCVECF class here?

So the FakeCVECF is so that we don't have to use validations that belong to CVECF. I kept getting hit with no content source migration errors because of this => https://github.com/Katello/katello/blob/master/app/models/katello/content_view_environment_content_facet.rb#L10

SmartProxy is nil during migration since its org scoped (even though the content_source_id exists). So when you save you hit this error.

@@ -211,7 +211,7 @@ def enabled_repos

#api :POST, "/environments/:environment_id/consumers", N_("Register a consumer in environment")
def consumer_create
host = Katello::RegistrationManager.process_registration(rhsm_params, find_content_view_environments)
host = Katello::RegistrationManager.process_registration(rhsm_params, content_view_environments)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be find_content_view_environments (see L#321 below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it back. I don't see this path being called from subman but may be some one who is curl posting to this URL directly will be calling this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, still the same issue:

[ActiveRecord::RecordInvalid] exception expected, not
Class: <ActiveModel::UnknownAttributeError>
Message: <"unknown attribute 'content_view' for Katello::ActivationKey.">

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, this is what it was originally before my PR:

    def consumer_create
      host = Katello::RegistrationManager.process_registration(rhsm_params, find_content_view_environments) # note second arg here, calls find_content_view_environments method which is defined later in the same file

      host.reload

      update_host_registered_through(host, request.headers)
      render :json => Resources::Candlepin::Consumer.get(host.subscription_facet.uuid)
    end

My PR had:

    def consumer_create
      content_view_environments = find_content_view_environments # content_view_environments is defined here
      if content_view_environments.any? { |cve| cve.content_view.generated_for_repository_export? }
        render json: {
          displayMessage: _("Cannot register with a content view generated for repository export"),
          errors: [_("Cannot register with a content view generated for repository export")]
        }, status: :bad_request
        return
      end

      host = Katello::RegistrationManager.process_registration(rhsm_params, content_view_environments) # second arg here now uses above definition

      host.reload

      update_host_registered_through(host, request.headers)
      render :json => Resources::Candlepin::Consumer.get(host.subscription_facet.uuid)
    end

And yours had:

    def consumer_create
      host = Katello::RegistrationManager.process_registration(rhsm_params, content_view_environments) # but content_view_environments wasn't defined as in mine

      host.reload

      update_host_registered_through(host, request.headers)
      render :json => Resources::Candlepin::Consumer.get(host.subscription_facet.uuid)
    end

So I wasn't suggesting that you need to reinstate my version of the change, just that you never FULLY reverted it to the original

That said, it seems the current issue in the tests is occurring regardless. I'm not sure what is the cause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[ActiveRecord::RecordInvalid] exception expected, not
Class: ActiveModel::UnknownAttributeError
Message: <"unknown attribute 'content_view' for Katello::ActivationKey.">

can you share the stack trace for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you share the stack trace for this.

I kicked off a re-run in CI of the failed tests to be sure:

 Error: Failure: test_0011_should not allow generated content view to be associated with activation key

[ActiveRecord::RecordInvalid] exception expected, not
Class: <ActiveModel::UnknownAttributeError>
Message: <"unknown attribute 'content_view' for Katello::ActivationKey.">
---Backtrace---
katello/test/models/activation_key_test.rb:114:in `block (2 levels) in <class:ActivationKeyTest>'
katello/test/models/activation_key_test.rb:113:in `block in <class:ActivationKeyTest>'
---------------

It looks like the test needs to try updating convent_view_environments instead

Author:    William Bradford Clark <[email protected]>
Co-authored-by: Partha Aji <[email protected]>

Ensures no Hosts remain on ContentViews generated for repository export.
If doing so would leave the Host a valid ContentView, the Default
Organization View is assigned to it.

Also implements additional filtering in the candlepin proxies controller,
and several additional tests.
Comment on lines +110 to +118
test "should not allow generated content view to be associated with activation key" do
generated_cv = FactoryBot.create(:katello_content_view, :generated_for => :repository_export, :organization => @dev_view.organization)

exception = assert_raises(ActiveRecord::RecordInvalid) do
@dev_key.update!(content_view: generated_cv)
end

assert_match(/Generated content views cannot be assigned to hosts or activation keys/, exception.message)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test "should not allow generated content view to be associated with activation key" do
generated_cv = FactoryBot.create(:katello_content_view, :generated_for => :repository_export, :organization => @dev_view.organization)
exception = assert_raises(ActiveRecord::RecordInvalid) do
@dev_key.update!(content_view: generated_cv)
end
assert_match(/Generated content views cannot be assigned to hosts or activation keys/, exception.message)
end
test "should not allow generated content view to be associated with activation key" do
generated_cv = FactoryBot.create(:katello_content_view, :generated_for => :repository_export, :organization => @dev_view.organization)
generated_cve = FactoryBot.create(:katello_content_view_environment, :content_view => generated_cv, :environment => katello_environments(:library))
exception = assert_raises(ActiveRecord::RecordInvalid) do
@dev_key.update!(content_view_environments: [generated_cve])
end
assert_match(/Generated content views cannot be assigned to hosts or activation keys/, exception.message)
end

Something along these lines looks necessary due to the changes in 746bda7#diff-d7dd2b8980210ef5f81a8ac4d2bfd2384210cf326a1b165a5b6869b0c661a241

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