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 #36569 - List only supported Job Templates on upstream #650

Merged
merged 1 commit into from Jul 25, 2023

Conversation

nofaralfasi
Copy link
Contributor

No description provided.

lib/foreman_ansible/engine.rb Outdated Show resolved Hide resolved
return super if Foreman::Plugin.find('foreman_theme_satellite').present?

@job_templates = super
@job_templates = @job_templates.where.not(name: JobTemplatesControllerExtensions::UNSUPPORTED_JOB_TEMPLATES)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if instead of this we update the db seed for the Ansible job templates?

    JobTemplate.without_auditing do
      Dir[File.join("#{ForemanAnsible::Engine.root}/app/views/foreman_ansible/"\
                    'job_templates/**/*.erb')].each do |template|
        sync = !Rails.env.test? && Setting[:remote_execution_sync_templates]
        template = JobTemplate.import_raw!(File.read(template),
                                           :default => true,
                                           :lock => true,
                                           :update => sync)
        template.organizations = organizations if template.present?
        template.locations = locations if template.present?
      end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If my understanding is correct, this code block is responsible for importing new templates and storing them in the database. However, as the Capsule Upgrade Playbook already exists in our database, we must create a migration to remove it, in addition to making changes in the code. The same process applies to handling the Smart Proxy Upgrade Playbook in the reverse scenario for the downstream.

On one hand, this approach ensures that unsupported job templates are not saved in the database, thereby avoiding the need for further code changes in the API. However, on the other hand, it requires the necessary migrations to delete data.

@ShimShtein, would you like to share your thoughts on this matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

However, as the Capsule Upgrade Playbook already exists in our database, we must create a migration to remove it, in addition to making changes in the code.

Do we need migration? Was it released to production already? In downstream it wasn't, not sure about the upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, it was released both downstream and upstream.
10.4.2 (downstream, foreman 3.5), 11.1.1 (foreman 3.6, foreman 3.7).

Copy link
Member

Choose a reason for hiding this comment

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

Removing the template in seeds/migrations seems more clean, but we will need a code that explicitly removes the unwanted template there.
Something like:

unsupported_templates = ['capsule_upgrade.erb']
unsupported_templates = ['smart_proxy_upgrade.erb'] if plugin.exists? 'foreman_theme_satellite'
Dir["templates/*.erb"].except(unsuppored_templates).each do |template|
  create_template(template)
end

JobTemplate.where(name: unsupported_template).delete_all

@stejskalleos stejskalleos self-assigned this Jul 20, 2023
@nofaralfasi nofaralfasi changed the title Fixes #36569 - List only supported Job Templates on upstream [WIP] Fixes #36569 - List only supported Job Templates on upstream Jul 25, 2023
@nofaralfasi nofaralfasi marked this pull request as draft July 25, 2023 06:56
@ShimShtein
Copy link
Member

Thank you! This looks much cleaner indeed.

db/seeds.d/75_job_templates.rb Outdated Show resolved Hide resolved
db/seeds.d/75_job_templates.rb Outdated Show resolved Hide resolved
Update the job_templates seed file to include only supported templates.
@nofaralfasi nofaralfasi changed the title [WIP] Fixes #36569 - List only supported Job Templates on upstream Fixes #36569 - List only supported Job Templates on upstream Jul 25, 2023
@nofaralfasi nofaralfasi marked this pull request as ready for review July 25, 2023 11:17
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

🍏 LGTM

Works well for both scenarios (upstream/downstream) and the code looks good too.
@ShimShtein if you don't have any other comments I'm 👍 to merge

@ShimShtein
Copy link
Member

LGTM.
I don't really like the fact that we have a piece of code (template in this case) that is not tested either in downstream or upstream, but any other solution would be much more complicated.

@nofaralfasi nofaralfasi merged commit 8679b8e into theforeman:master Jul 25, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants